fix: support comma-separated bootstrap servers.#634
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for comma-separated bootstrap_servers in the Rust client metadata initialization flow, enabling failover across multiple bootstrap endpoints (issue #585).
Changes:
- Introduces parsing of
bootstrap_serversas a comma-separated list with whitespace trimming and empty-entry rejection. - Updates metadata initialization to try bootstrap endpoints in order until one successfully returns cluster metadata.
- Adds unit tests covering single and comma-separated bootstrap formats plus empty-entry rejection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn parse_bootstrap_servers(bootstrap_servers: &str) -> Result<Vec<SocketAddr>> { | ||
| let mut addrs = Vec::new(); | ||
|
|
||
| for bootstrap in bootstrap_servers.split(',') { | ||
| let bootstrap = bootstrap.trim(); | ||
| if bootstrap.is_empty() { | ||
| return Err(Error::IllegalArgument { | ||
| message: format!( | ||
| "Invalid bootstrap servers '{bootstrap_servers}': empty bootstrap server" | ||
| ), | ||
| }); | ||
| } | ||
| addrs.push(Self::parse_bootstrap(bootstrap)?); | ||
| } | ||
|
|
||
| if addrs.is_empty() { | ||
| return Err(Error::IllegalArgument { | ||
| message: "No bootstrap servers configured".to_string(), | ||
| }); | ||
| } | ||
|
|
||
| Ok(addrs) | ||
| } |
| let socket_addresses = Self::parse_bootstrap_servers(bootstrap_servers)?; | ||
| let mut errors = Vec::new(); | ||
|
|
||
| for socket_address in socket_addresses { | ||
| let server_node = ServerNode::new( | ||
| -1, | ||
| socket_address.ip().to_string(), | ||
| socket_address.port() as u32, | ||
| ServerType::Unknown, | ||
| ); | ||
|
|
||
| match Self::fetch_cluster_from_bootstrap(&server_node, connections.clone()).await { | ||
| Ok(cluster) => return Ok(cluster), | ||
| Err(err) => errors.push(format!("{socket_address}: {err}")), | ||
| } | ||
| } | ||
|
|
||
| Err(Error::UnexpectedError { | ||
| message: format!( | ||
| "Unable to initialize cluster from bootstrap servers '{bootstrap_servers}': {}", | ||
| errors.join("; ") | ||
| ), | ||
| source: None, | ||
| }) |
| let addrs = Metadata::parse_bootstrap_servers("127.0.0.1:8080, localhost:9090, [::1]:7070") | ||
| .unwrap(); | ||
|
|
||
| assert_eq!(addrs.len(), 3); | ||
| assert_eq!(addrs[0].port(), 8080); | ||
| assert_eq!(addrs[1].port(), 9090); | ||
| assert_eq!(addrs[2].port(), 7070); |
| let addrs = Metadata::parse_bootstrap_servers("127.0.0.1:8080").unwrap(); | ||
| assert_eq!(addrs.len(), 1); | ||
| assert_eq!(addrs[0].port(), 8080); | ||
|
|
||
| // valid hostname |
leekeiabstraction
left a comment
leekeiabstraction
left a comment
There was a problem hiding this comment.
TY for the PR. Left a small comment.
| assert_eq!(addrs[0].port(), 8080); | ||
| assert_eq!(addrs[1].port(), 9090); | ||
| assert_eq!(addrs[2].port(), 7070); |
There was a problem hiding this comment.
Should assertion be done on the host/ip address value as well? Also applies on the previous test case.
|
@slfan1989 Thank you, can you please move this PR to fluss repo under fluss-rust folder? |
@fresh-borzoni Sure, thanks for letting me know. I’ll migrate these changes to the fluss-rust folder in the apache/fluss repository and open a new PR there. I’ll link the new PR here once it’s ready. |
Purpose
Linked issue: close #585
Support comma-separated
bootstrap_serversin the Rust client so users can configure multiple bootstrap endpoints for high availability.Brief change log
bootstrap_serversas a comma-separated list of bootstrap endpoints.Tests
Added unit tests.
API and Format
No public API or storage format changes.
Documentation
No documentation changes.