-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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-36069][runtime/rest] Extending job detail rest API to expose json stream graph #25798
base: master
Are you sure you want to change the base?
Conversation
Hi @JunRuiLee, can you help to review this PR? |
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.
Thanks @yuchen-ecnu for contributing, I've left two comments, PTAL!
@@ -101,6 +102,8 @@ void enableCheckpointing( | |||
|
|||
void setJsonPlan(String jsonPlan); | |||
|
|||
void setJsonStreamGraph(JsonStreamGraph jsonStreamGraph); |
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.
I prefer to retrieve the JSON of the stream graph from ExecutionPlanSchedulingContext
. For example, we could add a method getJsonStreamGraph
in ExecutionPlanSchedulingContext
. This context will return the JSON held by AdaptiveGraphManager
. When the stream graph is updated by the DefaultStreamGraphContext
, the AdaptiveGraphManager should update this JSON.
With this update, we could avoid manually passing this JSON to the execution graph and keep this change simple.
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.
Great tips! ExecutionPlanSchedulingContext
really helps a lot. I have remove the unexpected setJsonStreamGraph
methods from ExecutionGraph
in the revised version.
@@ -70,6 +70,8 @@ public enum ExecutionState { | |||
|
|||
RECONCILING, | |||
|
|||
PENDING, |
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.
How about handle pending-operators
in the JobDetailsHandler#createJobDetailsInfo
. This way, we wouldn't need to introduce any meaningless statuses to a common enum class.
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.
Agree with you. Since it’s not a valid status for stream jobs, how about including it in the serialized JSON stream graph (JsonPlanGenerator#generateJsonStreamGraph
)? This would help avoid adding interfaces like getPendingOperatorCounts()
to the ExecutionGraph
and ArchivedExecutionGraph
for processing in JobDetailsHandler#createJobDetailsInfo
. And it will be processed in frontend code.
02bc2ff
to
5ff6712
Compare
What is the purpose of the change
Brief change log
JobDetailHandler
to return the incrementally updated job graphs.Verifying this change
This change added tests and can be verified as follows:
JsonPlanGenerator
generatesJsonStreamGraph
works wellJobDetailHandler
returns Incremental JobGraph as expectedDoes this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation