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-19248] add missing lastJobExecutionResult assignment after job finished in ContexEnvironment #13396
base: master
Are you sure you want to change the base?
Conversation
… finished in ContexEnvironment
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 3eedbea (Fri Feb 19 07:20:30 UTC 2021) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
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 for the fix! I had a comment inline but I think it would also be good to add a test for this. The test should basically create a ContextEnvironment
, run a job, and then call getLastResult()
.
@@ -76,6 +76,7 @@ public JobExecutionResult execute(String jobName) throws Exception { | |||
final JobExecutionResult jobExecutionResult = getJobExecutionResult(jobClient); | |||
jobListeners.forEach(jobListener -> | |||
jobListener.onJobExecuted(jobExecutionResult, null)); | |||
this.lastJobExecutionResult = jobExecutionResult; |
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.
Instead of using a local variable jobExecutionResult
we could just always use the field lastJobExecutionResult
here.
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 got you.
Hi @aljoscha I can help with this here if all that’s missing is a test! Do you know where I can find examples of creating a |
What is the purpose of the change
This pull request fixes issue FLINK-19248 opened by myself.
Brief change log
Verifying this change
This change is a trivial rework / code cleanup without any test coverage. I add only one line of code in ContextEnvironment to fix the issue, it can be verified as issue FLINK-19248 says.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation