[rust][client] Support comma-separated bootstrap servers#3711
Conversation
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@slfan1989 Thank you for the PR, left some comments PTAL
One follow-up:Java retries a single bootstrap server 3x with backoff on retriable errors (tryToInitializeClusterWithRetries, the "bootstrap is recovering" case). The Rust client still has zero retries. Pre-existing, out of scope here, but worth an issue.
WDYT?
| match Self::fetch_cluster_from_bootstrap(&server_node, connections.clone()).await { | ||
| Ok(cluster) => return Ok(cluster), | ||
| Err(err) => { | ||
| errors.push(format!("{socket_address}: {err}")); |
There was a problem hiding this comment.
Can we add a warn! when a server fails and we move to the next one? Right now failover is invisible — nothing tells the operator that server 1 is down.
| errors: Vec<String>, | ||
| last_connection_error: Option<Error>, | ||
| ) -> Error { | ||
| if let Some(error) = last_connection_error { |
There was a problem hiding this comment.
If two servers fail with different errors, the user only sees the last one. E.g. auth error on A, timeout on B -> they see only the timeout. Can we log all of errors before returning?
|
|
||
| for bootstrap in bootstrap_servers.split(',') { | ||
| let bootstrap = bootstrap.trim(); | ||
| if bootstrap.is_empty() { |
There was a problem hiding this comment.
"host:9123," (trailing comma) fails here, but works in the Java client as it skips empty entries.
Should we do the same?
| ServerType::Unknown, | ||
| ); | ||
|
|
||
| match Self::fetch_cluster_from_bootstrap(&server_node, connections.clone()).await { |
There was a problem hiding this comment.
There's no test for the failover itself like server A fails, server B succeeds. Can we add one?
| }); | ||
| } | ||
|
|
||
| if bootstraps.is_empty() { |
There was a problem hiding this comment.
nit: Dead code as split(',') never returns zero entries, so "" hits the empty-entry error above first.
| match Self::fetch_cluster_from_bootstrap(&server_node, connections.clone()).await { | ||
| Ok(cluster) => return Ok(cluster), | ||
| Err(err) => { | ||
| errors.push(format!("{socket_address}: {err}")); |
There was a problem hiding this comment.
nit: Error messages mix labels: parse errors show what the user configured (bootstrap.raw), connection errors show the resolved IP. Shall we use raw in both?
2 participants
Purpose
Linked issue: close #3710
Brief change log
Support comma-separated bootstrap_servers in the Rust client so users can configure multiple bootstrap endpoints for high availability.
Brief change log
Tests
Added unit tests.
API and Format
No public API or storage format changes.
Documentation
No documentation changes.