swuferhong commented on code in PR #3390:
URL: https://github.com/apache/fluss/pull/3390#discussion_r3339806571
##########
fluss-client/src/main/java/org/apache/fluss/client/admin/FlussAdmin.java:
##########
@@ -139,13 +143,33 @@ public class FlussAdmin implements Admin {
private final AdminReadOnlyGateway readOnlyGateway;
private final MetadataUpdater metadataUpdater;
+ /**
+ * Single-thread executor that runs the metadata-refresh callback for
{@link
+ * RetryableGatewayClientProxy}.
+ */
+ private final ExecutorService refreshExecutor =
+ Executors.newFixedThreadPool(
+ 1, new
ExecutorThreadFactory("fluss-admin-metadata-refresh"));
+
public FlussAdmin(RpcClient client, MetadataUpdater metadataUpdater) {
- this.gateway =
+ AdminGateway rawGateway =
GatewayClientProxy.createGatewayProxy(
metadataUpdater::getCoordinatorServer, client,
AdminGateway.class);
- this.readOnlyGateway =
+ AdminGateway rawReadOnlyGateway =
GatewayClientProxy.createGatewayProxy(
metadataUpdater::getRandomTabletServer, client,
AdminGateway.class);
+ this.gateway =
+ RetryableGatewayClientProxy.createRetryableGatewayProxy(
+ rawGateway,
+ metadataUpdater::refreshClusterUntilAvailable,
Review Comment:
`invokeWithRetry` replays the same method/args on any `RetriableException`.
However, `AdminGateway` includes non-idempotent write operations such as
`createTable`, `dropTable`, and `createDatabase`.
Consider this dangerous scenario:
- The first createTable request actually succeeds on the server, but the
connection drops while the response is being returned (surfacing as a
NetworkException, which is a RetriableException).
- This triggers a metadata refresh + retry.
- The second createTable attempt then fails with TableAlreadyExistException.
The original `stale-metadata` problem (connection refused, request never
sent) is safe to retry. But the "request was delivered, only the response was
lost" case exposes non-idempotent semantics on retry.
##########
fluss-client/src/main/java/org/apache/fluss/client/admin/FlussAdmin.java:
##########
@@ -139,13 +143,33 @@ public class FlussAdmin implements Admin {
private final AdminReadOnlyGateway readOnlyGateway;
private final MetadataUpdater metadataUpdater;
+ /**
+ * Single-thread executor that runs the metadata-refresh callback for
{@link
+ * RetryableGatewayClientProxy}.
+ */
+ private final ExecutorService refreshExecutor =
+ Executors.newFixedThreadPool(
+ 1, new
ExecutorThreadFactory("fluss-admin-metadata-refresh"));
+
public FlussAdmin(RpcClient client, MetadataUpdater metadataUpdater) {
- this.gateway =
+ AdminGateway rawGateway =
GatewayClientProxy.createGatewayProxy(
metadataUpdater::getCoordinatorServer, client,
AdminGateway.class);
- this.readOnlyGateway =
+ AdminGateway rawReadOnlyGateway =
Review Comment:
This PR only includes a unit test for `RetryableGatewayClientProxy`.
The path where `FlussAdmin `wires the retry proxy into the real gateway has
no test coverage at all.
Given that the PR's goal is specifically recovery under address changes
during rolling upgrades, I'd suggest adding an ITCase to verify the convergence
behavior of: stale server → refresh → bootstrap fallback.
--
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]
