[#1231] feat(tez): Support remote spill in merge stage. - #1245
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1245 +/- ##
============================================
+ Coverage 53.70% 55.06% +1.35%
- Complexity 2615 2647 +32
============================================
Files 394 376 -18
Lines 22699 20653 -2046
Branches 1937 1971 +34
============================================
- Hits 12191 11372 -819
+ Misses 9766 8585 -1181
+ Partials 742 696 -46
... and 22 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| + usedMemory | ||
| + ", mapOutput=" | ||
| + mapOutput); | ||
| } else { |
There was a problem hiding this comment.
if (LOG.isDebugEnabled()) {
LOG.debug(
"closeInMemoryFile -> map-output of size: "
+ mapOutput.getSize()
+ ", inMemoryMapOutputs.size() -> "
+ inMemoryMapOutputs.size()
+ ", commitMemory -> "
+ commitMemory
+ ", usedMemory ->"
+ usedMemory
+ ", mapOutput="
+ mapOutput);
}
statsInMemLastLog.updateStats(mapOutput.getSize());
Maybe we could remove the else.
There was a problem hiding this comment.
if (LOG.isDebugEnabled()) { LOG.debug( "closeInMemoryFile -> map-output of size: " + mapOutput.getSize() + ", inMemoryMapOutputs.size() -> " + inMemoryMapOutputs.size() + ", commitMemory -> " + commitMemory + ", usedMemory ->" + usedMemory + ", mapOutput=" + mapOutput); } statsInMemLastLog.updateStats(mapOutput.getSize());Maybe we could remove the
else.
@jerqi
Here is to keep the same logic as tez.
In order to avoid excessive logs, info-level logs are printed every 30 seconds.
|
@lifeSo @bin41215 @zhuyaogai Could you help review this pr? |
| throw e; | ||
| } finally { | ||
| if (writer != null) { | ||
| writer.close(); |
There was a problem hiding this comment.
Why do we close this writer in the Line#179?
There was a problem hiding this comment.
Why do we close this writer in the Line#179?
@jerqi
Both TezMerger.merge and TezMerger.writeFile may throw exception. If throw exception, writer will not be closed.
By the way, both Line#179 and Line#181 seems useless. But by my debug, I found Line#180 must be called after Line#179. And we can not move Line#180 to finally block.
There was a problem hiding this comment.
Could you add some comments to explain why? It's a little weird.
There was a problem hiding this comment.
Could you add some comments to explain why? It's a little weird.
OK, I have comment it.
roryqi
left a comment
roryqi
left a comment
There was a problem hiding this comment.
LGTM, @lifeSo @bin41215 @zhuyaogai Any suggestion?
|
Merged to master. Thanks @zhengchenyu |
3 participants
What changes were proposed in this pull request?
Add new merger manager for remote spill.
Why are the changes needed?
In a cloud scenario, the storage of the machine that performs the job is very small. The merge phase often uses disk. The remote spill allows remote shuffle service job to use as little disk as possible.
Fix: #1231
Does this PR introduce any user-facing change?
How was this patch tested?
unit test, integration test, tez examples on cluster.