[#736] feat(storage): best effort to write same hdfs file when no race condition #744
Conversation
…no race condition
| // Use add() here because we are sure the capacity will not be exceeded. | ||
| // Note: add() throws IllegalStateException when queue is full. | ||
| queue.add(writeHandler); | ||
| // Use addFirst() here because we are sure the capacity will not be exceeded. |
There was a problem hiding this comment.
Could we add some comments to explain why we need to use method addFirst?
There was a problem hiding this comment.
Done. Add the javadoc for this class.
There was a problem hiding this comment.
I think you didn't address @jerqi's comment. A javadoc for this class is good.
But it still doesn't explain why we are using addFirst here..
And I believe the comment here is not up to date since it was for add() method.
There was a problem hiding this comment.
Emm. Yes, this comment is not for this. But I still think the javadoc is enough.
Codecov Report
@@ Coverage Diff @@
## master #744 +/- ##
============================================
+ Coverage 60.63% 62.88% +2.24%
- Complexity 1893 1901 +8
============================================
Files 238 224 -14
Lines 13000 11053 -1947
Branches 1090 1091 +1
============================================
- Hits 7883 6951 -932
+ Misses 4679 3725 -954
+ Partials 438 377 -61
... and 17 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @VisibleForTesting | ||
| public PooledHdfsShuffleWriteHandler(LinkedBlockingDeque<ShuffleWriteHandler> queue) { | ||
| this.queue = queue; | ||
| this.maxConcurrency = queue.size(); |
There was a problem hiding this comment.
nit: extra space.
| this.maxConcurrency = queue.size(); | |
| this.maxConcurrency = queue.size(); |
There was a problem hiding this comment.
Oh, sorry. I missed this thread.
Got it. I will fix this in another PR.
| // Use add() here because we are sure the capacity will not be exceeded. | ||
| // Note: add() throws IllegalStateException when queue is full. | ||
| queue.add(writeHandler); | ||
| // Use addFirst() here because we are sure the capacity will not be exceeded. |
There was a problem hiding this comment.
I think you didn't address @jerqi's comment. A javadoc for this class is good.
But it still doesn't explain why we are using addFirst here..
And I believe the comment here is not up to date since it was for add() method.
|
cc @zuston You seems that we miss some information from advancedxy. |
…no race condition (apache#744) ### What changes were proposed in this pull request? best effort to write same hdfs file when no race condition in `PooledHdfsShuffleWriteHandlerTest` ### Why are the changes needed? 1. Reduce the file number for one partition to reduce HDFS pressure. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? 1. UTs
What changes were proposed in this pull request?
best effort to write same hdfs file when no race condition in
PooledHdfsShuffleWriteHandlerTestWhy are the changes needed?
Does this PR introduce any user-facing change?
No.
How was this patch tested?