fhan688 commented on code in PR #3645:
URL: https://github.com/apache/fluss/pull/3645#discussion_r3585346154
##########
fluss-server/src/main/java/org/apache/fluss/server/DynamicConfigManager.java:
##########
@@ -40,21 +40,26 @@
import java.util.Map;
import java.util.Objects;
-/** Manager for dynamic configurations. */
+/**
+ * Manager for dynamic configurations.
+ *
+ * <p>Used by both the CoordinatorServer and the TabletServer. Every instance,
regardless of server
+ * role or leadership status, continuously applies dynamic config-change
notifications from
+ * ZooKeeper. In particular a standby CoordinatorServer keeps tracking changes
so that, once it is
+ * promoted to leader, its components (e.g. SASL credentials) already reflect
the latest config
+ * rather than a stale snapshot taken at startup.
+ */
public class DynamicConfigManager {
private static final Logger LOG =
LoggerFactory.getLogger(DynamicConfigManager.class);
private static final long CHANGE_NOTIFICATION_EXPIRATION_MS = 15 * 60 *
1000L;
private final DynamicServerConfig dynamicServerConfig;
private final ZooKeeperClient zooKeeperClient;
private final ZkNodeChangeNotificationWatcher configChangeListener;
- private final boolean isCoordinator;
- public DynamicConfigManager(
- ZooKeeperClient zooKeeperClient, Configuration configuration,
boolean isCoordinator) {
Review Comment:
> As discussed, please only allow the follower coordinator to subscribe to
ZooKeeper notifications. The leader already maintains the latest configuration
changes, so having it subscribe here may cause version rollback issues (e.g., A
→ B → A). When a follower transitions to become the leader, please disable the
subscription at that point.
Thanks @loserwang1024, you're right — done. I've reworked it so the leader
no longer consumes notifications:
- Added a `listeningEnabled` gate to `DynamicConfigManager`. The
notification handler no-ops when it's off, so a paused instance won't react to
ZooKeeper changes.
- `pauseListening()` is called in `CoordinatorServer#initCoordinatorLeader`
(on promotion) and `resumeListening()` in `cleanupCoordinatorLeader` (on
demotion). A standby/tablet server keeps listening as before.
- On demotion, `resumeListening()` also re-fetches once from ZooKeeper:
while the server was leader the watcher kept advancing its last-processed
sequence id but skipped the bodies, so past notifications won't replay — the
explicit re-sync picks up whatever the new leader changed meanwhile.
- Reverted the write-lock refactor from the previous revision
(`refreshDynamicConfig`/`updateAndPersistDynamicConfig`) back to the original
`updateDynamicConfig`. With the leader no longer listening, a single instance
only ever has one writer, so that locking is no longer needed.
I could reproduce the A → B → A rollback you described in a unit test (`SET
null` → `DELETE` restoring the default was rolled back to `null` by the
manager's own notification), which is now covered by
`testPausedManagerIgnoresNotificationsAndResumeReSyncs`. The failover IT is
extended to check the promoted leader keeps the value it tracked as standby.
PTAL.
--
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]
