slfan1989 commented on code in PR #8681:
URL: https://github.com/apache/hadoop/pull/8681#discussion_r3889126549
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/test/java/org/apache/hadoop/yarn/server/router/subcluster/TestMockRouter.java:
##########
@@ -73,18 +77,40 @@ public static void main(String[] args) throws YarnException
{
conf.set(YarnConfiguration.ROUTER_WEBAPP_ADDRESS,
getHostNameAndPort(pRouterWebAddressPort));
- RetryPolicy retryPolicy =
FederationStateStoreFacade.createRetryPolicy(conf);
-
+ // This class is launched as its own JVM by JavaProcess, and the parent
test
+ // terminates it with Process#destroy: SIGTERM on Linux and macOS, which
runs
+ // shutdown hooks. (On Windows destroy maps to TerminateProcess, which runs
+ // none, so there this is inert.) Router#main registers this hook itself;
+ // starting the Router directly, as here, skips it, so without this the JVM
+ // exits with the Router's services still running and none of them ever
+ // stopped.
+ ShutdownHookManager.get().addShutdownHook(
+ new CompositeServiceShutdownHook(router),
Router.SHUTDOWN_HOOK_PRIORITY);
router.init(conf);
router.start();
- FederationStateStore stateStore = (FederationStateStore)
- FederationStateStoreFacade.createRetryInstance(conf,
- YarnConfiguration.FEDERATION_STATESTORE_CLIENT_CLASS,
- YarnConfiguration.DEFAULT_FEDERATION_STATESTORE_CLIENT_CLASS,
- FederationStateStore.class, retryPolicy);
- stateStore.init(conf);
- FederationStateStoreFacade.getInstance().reinitialize(stateStore, conf);
+ // Starting the Router has already created and initialized the facade's
+ // store: RouterClientRMService#serviceStart builds a
+ // RouterDelegationTokenSecretManager whose constructor calls
+ // FederationStateStoreFacade#getInstance(Configuration). Building a second
+ // store here and handing it to reinitialize() would orphan that first one
-
+ // reinitialize swaps the reference, it does not close what it replaces -
so
+ // take the store the facade is already using.
+ FederationStateStore stateStore =
+ FederationStateStoreFacade.getInstance(conf).getStateStore();
+
+ // The facade holds this store but never closes it, so its ZooKeeper
Review Comment:
The ordering here assumes that `router.stop()` has stopped every user of the
federation state store.
However, `RouterClientRMService#serviceStart()` calls
`routerDTSecretManager.startThreads()`, while `serviceStop()` never calls
`stopThreads()`.
As a result, the `ExpiredTokenRemover` thread is still running when this
lower-priority hook closes the state store, and it may still access the facade
while rolling master keys or removing expired tokens. This leaves a
Router-owned thread outside the service lifecycle and makes shutdown racy.
Could we stop the delegation-token secret manager before closing the state
store, ideally in `RouterClientRMService#serviceStop()`?
If that production change is outside this JIRA, it should at least be
stopped explicitly by the mock Router shutdown logic before the state store is
closed.
--
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]
