-
Notifications
You must be signed in to change notification settings - Fork 13.1k
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-12101] Race condition when concurrently running uploaded jars via REST #8543
Conversation
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community 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:
|
return contextEnvironmentFactory == null ? | ||
createLocalEnvironment() : contextEnvironmentFactory.createExecutionEnvironment(); | ||
(contextEnvironmentFactoryThreadLocal.get() == null ? |
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.
Do we need to add contextEnvironmentFactoryThreadLocal
check in the function areExplicitEnvironmentsAllowed
(line 1284). I can't add a review there.
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 your review @klion26. Good catch and updated.
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 creating this PR @leesf. I had a comment about the precedence of the factory fields. Moreover, I think we need to do the same for the StreamExecutionEnvironment
if I'm not mistaken.
It would also be good if we could add a test for this fix. We could create a dummy ExecutionEnvironmentFactory
for this purpose.
return contextEnvironmentFactory == null ? | ||
createLocalEnvironment() : contextEnvironmentFactory.createExecutionEnvironment(); | ||
(contextEnvironmentFactoryThreadLocal.get() == null ? |
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 would suggest to first check the thread local variable and then fall back to the static variable. That way, it would not be possible to override the value of an ExecutionEnvironment
which has been set explicitly by from another thread.
@tillrohrmann Thanks for your review. Updated this PR to address your comments. |
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 addressing my comments @leesf. LGTM. The failing test case is unrelated. Merging this PR now.
…solveFactory ExecutionEnvironment#resolveFactory selects between the thread local and the global factory. This method is used by the ExecutionEnvironment as well as the StreamExecutionEnvironment. This closes apache#8543.
…solveFactory ExecutionEnvironment#resolveFactory selects between the thread local and the global factory. This method is used by the ExecutionEnvironment as well as the StreamExecutionEnvironment. This closes apache#8543.
What is the purpose of the change
Fix race condition when concurrently running uploaded jars via REST.
Brief change log
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (no)Documentation
cc @tillrohrmann