Skip to content
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

[#708] test: do not assume hostname of hdfs mini-cluster #709

Merged
merged 4 commits into from
Mar 10, 2023

Conversation

kaijchen
Copy link
Contributor

@kaijchen kaijchen commented Mar 10, 2023

What changes were proposed in this pull request?

Do not assume hostname in hdfs URL.
Also, let exception be thrown in ShuffleHdfsStorageUtilsTest to print verbose message.

Why are the changes needed?

Fix #708

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested manually

jerqi
jerqi previously approved these changes Mar 10, 2023
Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @kaijchen

@kaijchen kaijchen changed the title [#708] test: do not assume hdfs hostname is "localhost" [#708] test: do not assume hostname of hdfs mini-cluster Mar 10, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #709 (0fae8f6) into master (4068593) will increase coverage by 2.16%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master     #709      +/-   ##
============================================
+ Coverage     60.52%   62.69%   +2.16%     
  Complexity     1847     1847              
============================================
  Files           228      214      -14     
  Lines         12744    10780    -1964     
  Branches       1067     1067              
============================================
- Hits           7713     6758     -955     
+ Misses         4623     3674     -949     
+ Partials        408      348      -60     

see 14 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kaijchen kaijchen requested a review from jerqi March 10, 2023 13:06
@jerqi jerqi merged commit 1e605c7 into apache:master Mar 10, 2023
@kaijchen kaijchen deleted the fix-hdfs-test branch March 10, 2023 13:53
advancedxy pushed a commit to advancedxy/incubator-uniffle that referenced this pull request Mar 21, 2023
…e#709)

### What changes were proposed in this pull request?

Do not assume hostname in hdfs URL.
Also, let exception be thrown in `ShuffleHdfsStorageUtilsTest` to print verbose message.

### Why are the changes needed?

Fix apache#708 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Tested manually
xianjingfeng pushed a commit to xianjingfeng/incubator-uniffle that referenced this pull request Apr 5, 2023
…e#709)

### What changes were proposed in this pull request?

Do not assume hostname in hdfs URL.
Also, let exception be thrown in `ShuffleHdfsStorageUtilsTest` to print verbose message.

### Why are the changes needed?

Fix apache#708 

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Tested manually
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.

[Flaky Test] ShuffleKerberizedHdfsStorageUtilsTest failed when hostname is not "localhost"
3 participants