Savonitar commented on code in PR #28639:
URL: https://github.com/apache/flink/pull/28639#discussion_r4093188457
##########
flink-runtime/src/main/java/org/apache/flink/runtime/resourcemanager/ResourceManager.java:
##########
@@ -427,6 +430,22 @@ public CompletableFuture<RegistrationResponse>
registerJobMaster(
jobMasterIdFuture,
(JobMasterGateway jobMasterGateway, JobMasterId
leadingJobMasterId) -> {
if (Objects.equals(leadingJobMasterId,
jobMasterId)) {
+ // Register with the delegation token
manager first, so a
+ // provider failure rejects the
registration and the job does
+ // not start without the tokens it
requires. LinkageError is
+ // caught so a plugin classpath failure is
reported the same
+ // way.
+ try {
+
delegationTokenManager.registerJob(jobId, jobConfiguration);
+ } catch (Exception | LinkageError e) {
Review Comment:
> Why do we want to prepare for LinkageError?
The LinkageError catch is there so a failed registration doesn't leave job
state in the providers that already registered the job.
It doesn't swallow the error: registerJob rolls back, logs, and rethrows it
unchanged. Since aa1b641aa45c860456c67e835c7c53022e0dc1ca the manager also
doesn't track a job whose registration failed.
I also conducted experiments with a real JobManager JVM (session cluster,
both without HA and with ZooKeeper HA) using a provider whose registerJob()
throws NoClassDefFoundError, with and without the catch.
I see three options:
1. Keep the catch (current branch): a failed registration is rolled back on
all providers and logged at ERROR with the job and provider, and the error is
rethrown so the registration is rejected.
2. Remove the catch (the suggestion, if I understand it correctly): the
registration is rejected the same way and **the JobManager _stays up_**, but
nothing rolls back. Providers that already registered the job **keep its
state** until the job ends and the job timeout fires (HA), or until the
JobManager shuts down (no HA, or a job that keeps restarting). The manager no
longer logs the failure at ERROR. A process-level failure happens only if a
provider's token obtain later throws an Error because of that state (a crash
loop under HA).
3. Remove the catch and fail explicitly: escalate a LinkageError from
registerJob() with onFatalError, so any broken provider deployment fails the
JobManager, at the cost of the other jobs on a session cluster.
Please correct me if I'm missing something and appreciate if you can share
your opinion on these tradeoffs.
--
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]
