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-16234][tests] Fix unstable cases in StreamingJobGraphGeneratorTest #11187
[FLINK-16234][tests] Fix unstable cases in StreamingJobGraphGeneratorTest #11187
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 7f887f5 (Sat Feb 22 15:03:43 UTC 2020) 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:
|
If the test assertion is flakey, why not fix the test assertion instead? |
Thanks for reporting this issue and trying to fix it @cpugputpu . @cpugputpu do you want to fix it? |
Thanks for your valuable comments! I am sorry that my previous fix was not a good one. Now I fix it without changing StreamGraph.java. |
final JobVertex source2Vertex = verticesSorted.get(1); | ||
final JobVertex map1Vertex = verticesSorted.get(2); | ||
final JobVertex map2Vertex = verticesSorted.get(3); | ||
JobVertex source1Vertex = null, source2Vertex = null, map1Vertex = null, map2Vertex= 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.
we should not declare multiple variables in one line.
final JobVertex map2Vertex = verticesSorted.get(3); | ||
JobVertex source1Vertex = null, source2Vertex = null, map1Vertex = null, map2Vertex= null; | ||
for (int i = 0; i < 4; i++) | ||
{ |
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.
The left brace should be in the same line with the for statement.
for (int i = 0; i < 4; i++) | ||
{ | ||
JobVertex vertex = verticesSorted.get(i); | ||
if (vertex.getName().equals("Source: source1")) |
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 body should always be surrounded by braces.
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'd prefer to change the matching to if (vertex.getName().contains("source1"))
.
final JobVertex map1Vertex = verticesSorted.get(2); | ||
final JobVertex map2Vertex = verticesSorted.get(3); | ||
JobVertex source1Vertex = null, source2Vertex = null, map1Vertex = null, map2Vertex= null; | ||
for (int i = 0; i < 4; i++) |
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.
we could extract this matching block to a common method to avoid duplication. It can return a list of JobVertices in the expected order.
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 comments! I have updated the fix.
|
||
private List<JobVertex> getExpectedVerticesList(List<JobVertex> vertices) { | ||
List<JobVertex> verticesMatched = new ArrayList<JobVertex>(); | ||
List<String> ExpectedOrder = Arrays.asList("source1", "source2", "map1", "map2"); |
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.
the head letter of this variable should not be capitalized.
@@ -857,4 +860,17 @@ private static Method getSetResourcesMethodAndSetAccessible(final Class<?> clazz | |||
setResourcesMethod.setAccessible(true); | |||
return setResourcesMethod; | |||
} | |||
|
|||
private List<JobVertex> getExpectedVerticesList(List<JobVertex> vertices) { |
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.
this method can be static
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 all the comments.
The change looks good to me. Merging.
This PR aims to solve the issue presented here: https://issues.apache.org/jira/browse/FLINK-16234
What is the purpose of the change
The fix is to change the HashSet to LinkedHashSet to make the tests more stable.
Verifying this change
This change is already covered by existing tests, and it can pass them successfully.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation