New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-9211][REST] JarRunHandler submits job to Dispatcher via RPC #5903
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good! 👍 Just had one inline question.
}); | ||
|
||
CompletableFuture<Acknowledge> jobSubmissionFuture = jarUploadFuture.thenCompose(jobGraph -> { | ||
// we have to enable queued scheduling because slots will be allocated lazily |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this also what the thing sitting behind the REST endpoint does?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enabling queued scheduling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If by the thing sitting behind the REST endpoint
you mean the Dispatcher
, then no, it does not enable queued scheduling, but effectively relies on it being enabled. If you don't enable it beforehand in the client/handler the job submission will outright fail with an entirely unhelpful error message.
The Dispatcher
should probably enable it though, but that isn't really in the scope of this PR as it would also affect the RestClusterClient
which does the same thing.
Technically it may also be questionably as it would invalidate part of the JobGraph
API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gotcha. I think that's way off topic, so this is still good to merge
merging. |
What is the purpose of the change
This PR reworks the
JarRunHandler
to submit the job to the dispatcher via RPC, instead of setting up aRestClusterClient
and going through the client's job-submission routine.The reasoning is that the existing behavior was causing issues on kubernetes, and this change also removes a special-case as this was the only handler that actively sends out rest requests.
Brief change log
JarRunHandler
now has access toDispatcherGateway
JarRunHandler
now uploads jar and submits job via RPCVerifying this change
JarRunHandlerTest
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation