wuchong commented on code in PR #3539:
URL: https://github.com/apache/fluss/pull/3539#discussion_r3525217710
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorEventProcessor.java:
##########
@@ -587,6 +609,25 @@ private void onShutdown() {
tabletServerChangeWatcher.stop();
}
Review Comment:
Make offline-leader retry scheduling lazy instead of polling forever
The current implementation starts a fixed-delay task at coordinator startup
and enqueues `RetryOfflineLeaderEvent` every interval, even when there are no
offline replicas to retry. In the common healthy case this means the
coordinator wakes up forever, enters the event queue, scans an empty set, and
returns. This is lightweight per run, but it is still another always-on
background loop, and it becomes a pattern that is easy to copy for future
recovery paths.
Could we make the retry scheduler event-driven/lazy instead? For example:
1. Do not start the offline-leader retry task during
`CoordinatorEventProcessor.startup()`.
2. When `onReplicaBecomeOffline(...)` adds an offline marker, check whether
there are retryable offline replicas on live tablet servers.
3. If there is no existing scheduled retry, schedule a one-shot retry after
`coordinator.offline-leader.retry-interval`.
4. When the scheduled task fires, only enqueue one
`RetryOfflineLeaderEvent`; keep all `CoordinatorContext` reads/writes inside
the coordinator event thread.
5. In `processRetryOfflineLeader()`, clear the scheduled-future marker
before processing.
6. If the retry still fails, the later
`NotifyLeaderAndIsrResponseReceivedEvent` will mark the replica offline again
and schedule the next one-shot retry.
7. If the retry succeeds, no replica is marked offline again, so no further
retry is scheduled.
This keeps the same recovery behavior while avoiding permanent empty polling
in healthy clusters.
##########
fluss-server/src/main/java/org/apache/fluss/server/coordinator/CoordinatorEventProcessor.java:
##########
@@ -262,6 +269,18 @@ public CoordinatorEventProcessor(
this.rebalanceManager =
new RebalanceManager(
this, zooKeeperClient, coordinatorEventManager,
SystemClock.getInstance());
+ this.offlineLeaderRetryIntervalMs =
+
conf.get(ConfigOptions.COORDINATOR_OFFLINE_LEADER_RETRY_INTERVAL).toMillis();
+ if (offlineLeaderRetryIntervalMs <= 0) {
+ throw new IllegalArgumentException(
+ String.format(
+ "%s must be positive, but was %d ms.",
+
ConfigOptions.COORDINATOR_OFFLINE_LEADER_RETRY_INTERVAL.key(),
+ offlineLeaderRetryIntervalMs));
+ }
+ this.offlineLeaderRetryScheduler =
+ Executors.newSingleThreadScheduledExecutor(
Review Comment:
Reuse a shared scheduler for lightweight coordinator tasks
This change adds another component-owned `ScheduledExecutorService` just to
periodically enqueue a lightweight coordinator event. Threads are a precious
resource, and we should be deliberate before introducing one more scheduler per
feature. The coordinator side already has several similar lightweight
schedulers (`TableLifecycleThrottler`, `AutoPartitionManager`,
`RebalanceManager`, `LakeTableTieringManager`, `ProducerOffsetsManager`,
`KvSnapshotLeaseManager`), which makes it easy for future changes to keep
adding dedicated threads unchecked.
Could we follow the TabletServer pattern instead: let the server own a
shared `FlussScheduler`/background scheduler and pass it to coordinator
components that only need lightweight delayed or periodic tasks? At minimum,
this new retry logic should reuse an existing shared scheduler rather than
creating its own single-thread executor.
--
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]
