Savonitar commented on code in PR #28639:
URL: https://github.com/apache/flink/pull/28639#discussion_r3539289773
##########
flink-runtime/src/main/java/org/apache/flink/runtime/security/token/DefaultDelegationTokenManager.java:
##########
@@ -310,64 +361,127 @@ public void start(Listener listener) throws Exception {
this.listener = checkNotNull(listener, "Listener must not be null");
synchronized (tokensUpdateFutureLock) {
checkState(tokensUpdateFuture == null, "Manager is already
started");
+ stopped = false;
Review Comment:
Agreed on the rename and the early returns, implemented in 69438d6. However,
I used the name `running` because it is used more often in the codebase.
And another part I did differently: I set `running = true` before the inline
first cycle in `start()`, exactly where `stopped = false` sat, not at the end
of the method (if I understand your suggestion correctly). The first obtain
runs inline and checks the flag twice, at the top of startTokensUpdate() and
again in maybeScheduleRenewal() when it schedules the periodic renewal. With
the flag flipped only at the end of start(), the inline cycle sees not-running,
skips the whole first obtain, and never schedules the renewal.
startShouldBeIdempotent and startTokensUpdateShouldScheduleRenewal lock that
ordering in, both fail if the flag moves after the cycle.
Same reason inverted on the stop() side: `running = false` flips before the
per-job unregistration below it, so a re-obtain racing shutdown cannot schedule
a cycle for a session that is shutting down. That cleanup also runs
unconditionally (no early return) because ResourceManager calls stop() on the
failed-startup path even when start() never ran.
--
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]
