fhan688 opened a new pull request, #3645:
URL: https://github.com/apache/fluss/pull/3645
### Purpose
Linked issue: close #3625
On a `CoordinatorServer`, `DynamicConfigManager` fetched dynamic configs
from ZooKeeper only once at startup and then ignored every later config-change
notification: the notification handler returned early whenever `isCoordinator
== true`. As a result, a long-running **standby** coordinator's local config
drifted away from ZooKeeper.
When such a standby was later promoted to leader, its components were
configured from that stale local snapshot even though ZooKeeper already held
newer values. The most impactful case is SASL/PLAIN multi-user credentials
(`security.sasl.plain.credentials`, hot-reloaded by `FlussProtocolPlugin` and
registered on both server roles): after failover, the new leader could
authenticate clients against a frozen credential set — a dynamically added user
would be rejected, and a dynamically **removed** user would still be accepted.
This PR makes standby coordinators continuously apply dynamic config-change
notifications, exactly like tablet servers do, so a promoted leader always
reflects the latest config.
### Brief change log
- `DynamicConfigManager`: remove the `isCoordinator` early-return in the
change-notification handler, and drop the now-dead `isCoordinator` field and
constructor parameter. Both server roles now apply notifications.
- Fix a concurrency issue that this change surfaces on the leader, which now
both edits configs (via `alterConfigs`) and listens for notifications.
Previously the notification path fetched from ZooKeeper **outside** the write
lock, and `alterConfigs` applied-then-persisted without holding the lock across
both steps, so a stale fetch could be applied on top of a newer,
already-committed change. `DynamicServerConfig` now exposes:
- `refreshDynamicConfig(configSupplier, skipErrorConfig)` — fetch and
apply atomically under the write lock (used by `startup()` and the notification
handler);
- `updateAndPersistDynamicConfig(configs, persistAction)` — apply locally
and persist to ZooKeeper atomically under the write lock (used by
`alterConfigs`).
- Update `CoordinatorServer` and `TabletServer` to the two-argument
`DynamicConfigManager` constructor.
### Tests
-
`DynamicConfigChangeTest#testTracksConfigChangeNotificationsFromOtherWriter`
(new): a manager applies a config change written to ZooKeeper by another writer
— the core behavior the fix enables. Verified it fails without the fix
(`expected 3 but was 0`).
-
`CoordinatorHighAvailabilityITCase#testStandbyTracksDynamicConfigAndNewLeaderUsesItAfterFailover`
(new): alter a dynamic config on the leader, assert the standby tracks it,
kill the leader's ZK session to trigger failover, and assert the promoted
leader still uses the up-to-date value.
Verified it fails without the fix ("Standby coordinator did not apply
dynamic config change while standby").
- Updated the existing `DynamicConfigManager` construction call sites in
`DynamicConfigChangeTest` and made each test close its manager after use
(previously leaked notification watchers, now material because coordinators
react to notifications).
- Full runs pass: `DynamicConfigChangeTest` (26),
`CoordinatorHighAvailabilityITCase` (6), and the server startup/election suites
(`CoordinatorServerTest`, `TabletServerTest`, `CoordinatorServerElectionTest`).
### API and Format
No public API or storage-format changes. `DynamicConfigManager`'s
constructor signature changed, but it is an internal server class. No RPC
message or ZooKeeper znode-format changes.
### Documentation
No. This is a bug fix with no new user-facing feature or configuration.
--
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]
