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

Result of mkdirs() is ignored in LocalFileWriteHandler#createBasePath() #537

Merged
merged 5 commits into from
Feb 2, 2023

Conversation

kaijchen
Copy link
Contributor

@kaijchen kaijchen commented Feb 1, 2023

What changes were proposed in this pull request?

Fix error handling in LocalFileWriteHandler#createBasePath().
Throw a RssException in case of failure.

Why are the changes needed?

The return value of baseFolder.mkdirs() was ignored.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

CI.

@codecov-commenter
Copy link

codecov-commenter commented Feb 1, 2023

Codecov Report

Merging #537 (1174367) into master (721b22c) will increase coverage by 0.03%.
The diff coverage is 20.00%.

@@             Coverage Diff              @@
##             master     #537      +/-   ##
============================================
+ Coverage     60.23%   60.27%   +0.03%     
  Complexity     1785     1785              
============================================
  Files           205      205              
  Lines         11557    11556       -1     
  Branches       1042     1041       -1     
============================================
+ Hits           6961     6965       +4     
+ Misses         4189     4185       -4     
+ Partials        407      406       -1     
Impacted Files Coverage Δ
...le/storage/handler/impl/LocalFileWriteHandler.java 77.08% <20.00%> (+1.57%) ⬆️
...ategy/storage/AppBalanceSelectStorageStrategy.java 72.00% <0.00%> (-4.00%) ⬇️
...y/kubernetes/operator/pkg/webhook/inspector/rss.go 47.70% <0.00%> (ø)
...he/uniffle/server/buffer/ShuffleBufferManager.java 83.98% <0.00%> (+0.65%) ⬆️
...bernetes/operator/pkg/controller/controller/rss.go 33.22% <0.00%> (+0.74%) ⬆️

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

@kaijchen kaijchen changed the title Fix error handling in LocalFileWriteHandler#createBasePath() Throw exception when failed to create shuffle base dir Feb 1, 2023
try {
Files.createDirectories(Paths.get(basePath));
} catch (IOException e) {
throw new RuntimeException("Failed to create shuffle folder: " + basePath, e);
Copy link
Member

Choose a reason for hiding this comment

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

How about RssException ?

@kaijchen kaijchen changed the title Throw exception when failed to create shuffle base dir Result of mkdirs() is ignored in LocalFileWriteHandler#createBasePath() Feb 2, 2023
@kaijchen kaijchen added the bug Something isn't working label Feb 2, 2023
@kaijchen kaijchen merged commit 6f66f10 into apache:master Feb 2, 2023
@kaijchen kaijchen deleted the createBasePath branch February 2, 2023 09:19
@kaijchen
Copy link
Contributor Author

kaijchen commented Feb 2, 2023

Thanks @advancedxy and @zuston for the review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants