FrankChen021 commented on code in PR #20800:
URL: https://github.com/apache/druid/pull/20800#discussion_r4429463441
##########
extensions-core/kubernetes-extensions/src/main/java/org/apache/druid/k8s/discovery/LeaderElectorAsyncWrapper.java:
##########
@@ -103,8 +103,14 @@ public void close()
try {
futureRef.get().cancel(true);
executor.shutdownNow();
- if (!executor.awaitTermination(10, TimeUnit.SECONDS)) {
- LOGGER.warn("Failed to terminate [%s] executor.",
this.getClass().getSimpleName());
+ if (executor.awaitTermination(10, TimeUnit.SECONDS)) {
+ // Release the lock after the run loop has exited.
+ k8sLeaderElector.close();
+ } else {
+ LOGGER.warn(
+ "Failed to terminate [%s] executor. Not releasing the lock; it
will expire after the lease duration.",
Review Comment:
[P2] Ensure elector shutdown completes after timeout
**Finding:** When the outer executor cannot terminate within 10 seconds, or
the waiting lifecycle thread is interrupted, this method only logs or restores
the interrupt and never invokes k8sLeaderElector.close(). The Kubernetes
elector worker executors and ConfigMap lock therefore remain open; shutdown can
delay the next leader by the lease duration and leave non-daemon workers
running.
**Suggestion:** Arrange an eventual underlying-elector close after the
wrapper task finishes, including timeout and interruption paths, while
preserving interrupt status and avoiding release before stop callbacks complete.
##########
server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java:
##########
@@ -401,9 +401,15 @@ public void stop()
return;
}
- coordLeaderSelector.unregisterListener();
- metadataManager.stopCache();
started = false;
Review Comment:
[P2] Prevent late start hooks from relatching leadership
**Finding:** The Kubernetes client dispatches startLeadingHook on a separate
hook worker. If unregister interrupts the foreground run before that queued
hook executes, stopLeadingHook observes leader=false and returns; close then
drains the queued start hook, which sets the selector leader flag true before
this started=false guard returns. DruidCoordinator.isLeader() delegates
directly to that flag, so the stopped coordinator can still report leadership
after another pod owns the lock.
**Suggestion:** Serialize start and stop notifications or add a
selector-level stopping state that prevents queued start hooks from setting
leadership after unregister and clears the local leader flag before shutdown
completes.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
