Savonitar commented on code in PR #28639:
URL: https://github.com/apache/flink/pull/28639#discussion_r3539459138
##########
flink-runtime/src/main/java/org/apache/flink/runtime/security/token/DefaultDelegationTokenManager.java:
##########
@@ -416,13 +531,114 @@ long calculateRenewalDelay(Clock clock, long
nextRenewal) {
return renewalDelay;
}
+ @VisibleForTesting
+ void setClock(Clock clock) {
+ this.clock = clock;
+ }
+
/** Stops re-occurring token obtain task. */
@Override
public void stop() {
LOG.info("Stopping credential renewal");
- stopTokensUpdate();
+ synchronized (tokensUpdateFutureLock) {
+ // Mark stopped, cancel the pending cycle, and reset on-demand
re-obtain bookkeeping
+ // atomically, so a concurrent reobtainDelegationTokens() cannot
leave a live future
+ // orphaned after stop and a later start() does not inherit stale
state.
+ stopped = true;
+ stopTokensUpdate();
+ reobtainScheduled = false;
+ lastReobtainAtMillis = NO_PREVIOUS_REOBTAIN;
+ }
+
+ for (DelegationTokenProvider provider :
delegationTokenProviders.values()) {
Review Comment:
> Start called from another thread and start some/all of them
Providers have no start/restart hooks: init() runs once in the manager
constructor, and start() only drives the obtain logic, so there is nothing
start() could call to re-start a provider mid stop. An obtain cycle overlapping
stop() is possible, but that overlap is already covered by the stop() javadoc.
But another issue is possible. The manager instance is reused across
leadership sessions, so it is possible to have stop() then start() on the same
manager with the same provider instances and no re-init. A provider that closes
its resources in stop() (as the javadoc tells it to) is broken in the next
leadership term. The SPI javadoc says stop() is "called once during manager
shutdown" and the wiring does not honor that.
I see 2 options (taking into account the existing architecture/design):
1. Fan out provider.stop() only at process shutdown and let manager stop()
just stop scheduling. Providers keep a single init-to-stop lifecycle, matching
their single init. Pitfall: the only caller of manager.stop() today is
RM.onStop, which fires on revoke and shutdown alike and cannot tell them apart,
so this needs a separate teardown entry point wired into ClusterEntrypoint and
MiniCluster shutdown.
Or
2. Keep per-session stop() but redefine the contract: stop() may be followed
by another start(), so providers must release only per-run resources and
re-acquire lazy. Smaller diff, but it changes the agreed "called once" approach
and pushes restart-stuff into every provider implementation.
I lean towards option 1 because it keeps the provider lifecycle symmetric
with its single init and keeps the documented FLIP contract.
I have a Disabled test in the branch
(stopShouldKeepProvidersUsableForSubsequentStart) encoding option 1, to be
enabled once we discuss this.
WDYT?
--
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]
