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 55b0566. 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
provider.stop() fan-out, so a re-obtain racing shutdown cannot dispatch a cycle
onto providers mid-teardown, and the provider loop stays outside the 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]
