[FLINK-9787]Change ExecutionConfig#getGlobalJobParameters to return a… #8175
Conversation
|
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit 36cd7b9 (Fri Aug 30 07:58:35 UTC 2019) 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.
Thanks for opening this PR, @tianchen92. The PR LGTM.
However, I find another potential issue, which is not introduced by this PR.
GlobalJobParameters is designed to be an abstract class (according to the comments), but is actually implemented with a default toMap method. Therefore, it fails to guarantee that all classes that extend GlobalJobParameters implement proper toMap method. I would suggest to make GlobalJobParameters an interface, and introduce a singleton class EmptyGlobalJobParameters as the default.
I think we can either address this issue in this PR if you would like to, or merge this PR as it is and create another ticket regarding this. What do you think?
| @@ -741,6 +741,9 @@ public boolean isSysoutLoggingEnabled() { | |||
| } | |||
|
|
|||
| public GlobalJobParameters getGlobalJobParameters() { | |||
There was a problem hiding this comment.
Nice to mark it as @NonNull now.
There was a problem hiding this comment.
I think the community agreed that all return values which are not annotated with @Nullable are meant to be implicitly @NonNull. Hence theres should be no need for it.
| @Test | ||
| public void testGlobalParameters() { | ||
| final ExecutionConfig config = new ExecutionConfig(); | ||
|
|
There was a problem hiding this comment.
nit: maybe we get GlobalJobParameters empty = config.getGlobalJobParameters() first and then do the assertion.
There was a problem hiding this comment.
Thanks for this fix @tianchen92. I had some more comments how we could simplify the code further. Please take a look.
flink-core/src/main/java/org/apache/flink/api/common/ExecutionConfig.java
Outdated
Show resolved
Hide resolved
flink-core/src/test/java/org/apache/flink/api/common/ExecutionConfigTest.java
Outdated
Show resolved
Hide resolved
|
Thanks a lot for all your comments. @xintongsong I am fine if you would like to create another issue. |
There was a problem hiding this comment.
Thanks for this fix @tianchen92. Your changes look good to me modulo the added equals method. I'll remove this change and if Travis gives green light, then I'll merge this PR.
| @Override | ||
| public boolean equals(Object obj) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Why do we need this? If this is not needed, then please revert.
There was a problem hiding this comment.
I now understand why we need it.
There was a problem hiding this comment.
can we document the answer?
There was a problem hiding this comment.
The equals method is needed for the equals method of the ExecutionConfig which was not needed before because the field was null.
… a non null value
tillrohrmann
force-pushed
the
FLINK-9787
branch
from
d8ed435
to
36cd7b9
Compare
What is the purpose of the change
related to FLINK-9787. Change ExecutionConfig#getGlobalJobParameters to return an instance of GlobalJobParameters instead of null if no custom globalJobParameters are set yet
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation