-
Notifications
You must be signed in to change notification settings - Fork 145
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
[MINOR] refactor: use general method to check the remote storage existence #980
Conversation
Codecov Report
@@ Coverage Diff @@
## master #980 +/- ##
============================================
+ Coverage 53.12% 56.69% +3.57%
+ Complexity 2480 2201 -279
============================================
Files 375 321 -54
Lines 20347 14150 -6197
Branches 1725 1296 -429
============================================
- Hits 10810 8023 -2787
+ Misses 8866 5691 -3175
+ Partials 671 436 -235
... and 54 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
LGTM
lgtm, but looks like the test for tez is failed? |
Could you help check the Tez unit test failure? @lifeSo I think this is not related with this PR. |
@lifeSo Could you take a look at the failure test? If it is a flaky test, you can create an issue to track it and try to solve it. |
@@ -62,7 +62,7 @@ public static RemoteStorageInfo fetchRemoteStorage( | |||
String storageType, | |||
ShuffleWriteClient shuffleWriteClient) { | |||
RemoteStorageInfo remoteStorage = defaultRemoteStorage; | |||
if (requireRemoteStorage(storageType)) { | |||
if (StorageType.withRemoteStorage(StorageType.valueOf(storageType))) { |
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.
Check storageType != null
here?
The test fails because storageType
is null
.
…rTest (#1015) ### What changes were proposed in this pull request? Add storage type configs in TezRemoteShuffleManagerTest ### Why are the changes needed? StorageType defaults to null, which blocks #980. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Tested with #980.
@zhengchenyu @lifeSo @bin41215 Could you help take a look at the failure case? |
Seems the failed test is caused by |
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.
LGTM
What changes were proposed in this pull request?
In #259 , we introduce some general method to check the storage types. So
this patch is to refactor the remaining part of this.
Why are the changes needed?
Refactor to optimize the remote storage checking.
Does this PR introduce any user-facing change?
No.
How was this patch tested?