-
Notifications
You must be signed in to change notification settings - Fork 154
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
[#2388] improvement(test): mr, tez, spark integration test use random port #2397
Conversation
Test Results 3 011 files +32 3 011 suites +32 6h 43m 59s ⏱️ + 15m 41s Results for commit eb2df4d. ± Comparison against base commit b7969b4. This pull request removes 3 and adds 4 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
createCoordinatorServer(coordinatorConf); | ||
ShuffleServerConf grpcShuffleServerConf = getShuffleServerConf(ServerType.GRPC); | ||
createMockedShuffleServer(grpcShuffleServerConf); | ||
storeCoordinatorConf(coordinatorConf); |
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.
storeCoordinatorConf
storeMockShuffleServerConf
storeMockShuffleServerConf
startServersWithRandomPorts
looks ok, but is the code above a pattern? Should there be a method defined like startServersWithRandomPorts
with type parameters CoordinatorConf
and List<ShuffleServerConf>
? This pattern seems to have some repetition.
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.
There are quite a amount of cases,that the coordinator and shuffleServer assemble configuration in different place, it's better use fine-grained method to control
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.
Got ~
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.
+1, LGTM
@jerqi Could you take another look?
Merged into master. Thanks @summaryzb and @jerqi |
What changes were proposed in this pull request?
AccessClusterTest
Why are the changes needed?
Fix: #2388
Does this PR introduce any user-facing change?
No.
How was this patch tested?
UT