Copilot commented on code in PR #307:
URL: https://github.com/apache/fluss-rust/pull/307#discussion_r2802913067
##########
crates/fluss/src/client/metadata.rs:
##########
@@ -273,4 +281,18 @@ mod tests {
let cluster = metadata.get_cluster();
assert!(cluster.get_tablet_server(1).is_none());
}
+
+ #[test]
+ fn parse_bootstrap_variants() {
+ // valid IP
+ let addr = Metadata::parse_bootstrap("127.0.0.1:8080").unwrap();
+ assert_eq!(addr.port(), 8080);
+
+ // valid hostname
+ let addr = Metadata::parse_bootstrap("localhost:9090").unwrap();
+ assert_eq!(addr.port(), 9090);
+
+ // invalid input
Review Comment:
The test case "invalid_address" verifies that an invalid address returns an
error, but it would be more robust to also check for specific edge cases like:
addresses without ports (e.g., "localhost"), addresses with invalid ports
(e.g., "localhost:99999"), IPv6 addresses (e.g., "[::1]:8080"), and empty
strings. These cases would ensure the function handles malformed input
correctly.
```suggestion
// valid IPv6 address
let addr = Metadata::parse_bootstrap("[::1]:8080").unwrap();
assert_eq!(addr.port(), 8080);
// invalid input: missing port
assert!(Metadata::parse_bootstrap("localhost").is_err());
// invalid input: out-of-range port
assert!(Metadata::parse_bootstrap("localhost:99999").is_err());
// invalid input: empty string
assert!(Metadata::parse_bootstrap("").is_err());
// invalid input: nonsensical address
```
##########
crates/fluss/src/client/metadata.rs:
##########
@@ -44,13 +44,21 @@ impl Metadata {
})
}
+ fn parse_bootstrap(boot_strap: &str) -> Result<SocketAddr> {
+ let addr = boot_strap
+ .to_socket_addrs()
+ .map_err(|e| Error::IllegalArgument {
+ message: format!("Invalid bootstrap address '{boot_strap}':
{e}"),
+ })?
+ .next()
+ .ok_or_else(|| Error::IllegalArgument {
+ message: format!("Unable to resolve bootstrap address
'{boot_strap}'"),
+ })?;
+ Ok(addr)
+ }
Review Comment:
The method uses `to_socket_addrs()` which performs DNS resolution
synchronously, blocking the current thread. Since this method is called from
`init_cluster` (an async function), this DNS lookup could block the executor
thread, which is problematic in async contexts. Consider using async DNS
resolution instead, such as `tokio::net::lookup_host()`, to avoid blocking the
async runtime.
##########
crates/fluss/src/client/metadata.rs:
##########
@@ -44,13 +44,21 @@ impl Metadata {
})
}
+ fn parse_bootstrap(boot_strap: &str) -> Result<SocketAddr> {
+ let addr = boot_strap
+ .to_socket_addrs()
+ .map_err(|e| Error::IllegalArgument {
+ message: format!("Invalid bootstrap address '{boot_strap}':
{e}"),
+ })?
+ .next()
+ .ok_or_else(|| Error::IllegalArgument {
+ message: format!("Unable to resolve bootstrap address
'{boot_strap}'"),
+ })?;
Review Comment:
When `to_socket_addrs()` resolves a hostname to multiple IP addresses, using
`.next()` arbitrarily selects the first resolved address. This could lead to
inconsistent behavior across DNS resolutions and doesn't provide predictable
IPv4 vs IPv6 preference. Consider documenting this behavior or implementing a
more deterministic selection strategy (e.g., preferring IPv4 or allowing
configuration of address family preference).
```suggestion
// Resolve all socket addresses and deterministically choose one.
let mut addrs = boot_strap.to_socket_addrs().map_err(|e|
Error::IllegalArgument {
message: format!("Invalid bootstrap address '{boot_strap}':
{e}"),
})?;
// Prefer IPv4 addresses; if none are available, fall back to the
first IPv6.
let mut ipv6_candidate: Option<SocketAddr> = None;
while let Some(addr) = addrs.next() {
if addr.is_ipv4() {
return Ok(addr);
}
if ipv6_candidate.is_none() {
ipv6_candidate = Some(addr);
}
}
let addr = ipv6_candidate.ok_or_else(|| Error::IllegalArgument {
message: format!("Unable to resolve bootstrap address
'{boot_strap}'"),
})?;
```
##########
crates/fluss/src/client/metadata.rs:
##########
@@ -44,13 +44,21 @@ impl Metadata {
})
}
+ fn parse_bootstrap(boot_strap: &str) -> Result<SocketAddr> {
Review Comment:
The parameter name uses an underscore ("boot_strap") instead of the standard
compound word "bootstrap" used elsewhere in the codebase (see field name at
line 34, method parameter at line 38). This inconsistency makes the naming
confusing. The parameter should be renamed to "bootstrap" for consistency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
