Copilot commented on code in PR #369:
URL: https://github.com/apache/fluss-rust/pull/369#discussion_r2870433540
##########
crates/fluss/src/client/admin.rs:
##########
@@ -36,6 +36,7 @@ use std::collections::{HashMap, HashSet};
use std::sync::Arc;
use tokio::task::JoinHandle;
+#[derive(Clone)]
Review Comment:
Adding `#[derive(Clone)]` to the public `FlussAdmin` type expands the public
API surface and implicitly documents that cloning is a supported operation.
Since a clone will share the same underlying `ServerConnection` (it’s an
`Arc`), this can be surprising if callers expect an independent/fresh
connection. If cloning isn’t required for this PR, consider removing it; if it
is required, consider documenting the clone semantics explicitly (or returning
`Arc<FlussAdmin>` instead of relying on `Clone`).
```suggestion
```
##########
crates/fluss/src/client/connection.rs:
##########
@@ -60,6 +60,9 @@ impl FlussConnection {
}
pub async fn get_admin(&self) -> Result<FlussAdmin> {
+ // Create a new FlussAdmin on each call so that it can obtain a fresh
+ // coordinator connection from RpcClient and recover from transient
+ // connection failures.
Review Comment:
The new comment implies a “fresh coordinator connection” on each
`get_admin()` call, but `FlussAdmin::new()` delegates to
`RpcClient::get_connection()` which will typically return an existing cached
connection when it isn’t poisoned. Consider rewording to reflect that creating
a new `FlussAdmin` allows re-acquiring a connection when the cached one becomes
poisoned, rather than guaranteeing a fresh connection every time.
```suggestion
// Create a new FlussAdmin on each call so it can (re-)acquire a
healthy
// coordinator connection from RpcClient when needed (e.g., after a
// poisoned or failed connection), while typically reusing cached
// connections under normal operation.
```
##########
crates/fluss/src/client/connection.rs:
##########
@@ -60,6 +60,9 @@ impl FlussConnection {
}
pub async fn get_admin(&self) -> Result<FlussAdmin> {
+ // Create a new FlussAdmin on each call so that it can obtain a fresh
+ // coordinator connection from RpcClient and recover from transient
+ // connection failures.
FlussAdmin::new(self.network_connects.clone(),
self.metadata.clone()).await
}
Review Comment:
`get_admin()` is still constructing a new `FlussAdmin` on every call and
there’s no caching/check here, which doesn’t align with the PR
title/description (and issue #319) about caching the admin instance in
`FlussConnection`. Either implement an actual cached admin (e.g.,
`OnceCell`/`RwLock<Option<...>>` with async init) or update the PR scope/title
and remove the issue-closing reference if the intended behavior is to not cache.
--
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]
