[MINOR] refactor: use general method to check the remote storage existence by zuston · Pull Request #980 · apache/uniffle · GitHub
Skip to content

[MINOR] refactor: use general method to check the remote storage existence#980

Merged
kaijchen merged 3 commits into
apache:masterfrom
zuston:x-master
Jul 18, 2023
Merged

[MINOR] refactor: use general method to check the remote storage existence#980
kaijchen merged 3 commits into
apache:masterfrom
zuston:x-master

Conversation

@zuston

@zuston zuston commented Jun 28, 2023

Copy link
Copy Markdown
Member

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?

  1. Existing UTs

@zuston zuston requested a review from kaijchen June 28, 2023 03:27
@codecov-commenter

codecov-commenter commented Jun 28, 2023

Copy link
Copy Markdown

kaijchen
kaijchen previously approved these changes Jun 28, 2023

@kaijchen kaijchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@roryqi roryqi changed the title refactor: use general method to check the remote storage existence [MINOR] refactor: use general method to check the remote storage existence Jun 29, 2023
@advancedxy

Copy link
Copy Markdown
Contributor

lgtm, but looks like the test for tez is failed?

@zuston

zuston commented Jun 29, 2023

Copy link
Copy Markdown
Member Author

Could you help check the Tez unit test failure? @lifeSo I think this is not related with this PR.

@roryqi

roryqi commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

@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.

@roryqi

roryqi commented Jul 14, 2023

Copy link
Copy Markdown
Contributor

@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.

It seems a flaky test. Let crease an flaky test issue for @lifeSo . Let merge this pr first. Thanks all.

ShuffleWriteClient shuffleWriteClient) {
RemoteStorageInfo remoteStorage = defaultRemoteStorage;
if (requireRemoteStorage(storageType)) {
if (StorageType.withRemoteStorage(StorageType.valueOf(storageType))) {

Copy link
Copy Markdown
Member

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.

roryqi pushed a commit that referenced this pull request Jul 17, 2023
…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.
@roryqi

roryqi commented Jul 18, 2023

Copy link
Copy Markdown
Contributor

@zhengchenyu @lifeSo @bin41215 Could you help take a look at the failure case?

@kaijchen

Copy link
Copy Markdown
Member

@kaijchen kaijchen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kaijchen kaijchen merged commit e5e1b30 into apache:master Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants