[#2740] improvement(server) Just check block count while checking commit result#2742
Conversation
Test Results 3 162 files + 7 3 162 suites +7 6h 50m 7s ⏱️ -34s For more details on these failures, see this check. Results for commit 34a8fd9. ± Comparison against base commit b98b488. |
|
Count may not be correct if we have the duplicated data or the duplicated commit operation. |
| shuffleTaskInfos.computeIfAbsent(appId, x -> new ShuffleTaskInfo(appId)); | ||
| long size = 0L; | ||
| // With memory storage type should never need cachedBlockIds, | ||
| int blockCount = spbs.length - shufflePartitionedData.getDuplicateBlockCount(); |
There was a problem hiding this comment.
Duplicated blocks will not be countd. @roryqi
Furthermore, based on the current code, the same block will not be committed twice.
There was a problem hiding this comment.
Actually, the counter may be not accurate. We have too many branches. It is hard to prove it.
There was a problem hiding this comment.
Why do we need commit the data? If we use memory mode, we can avoid committing the blocks.
There was a problem hiding this comment.
Why do we need commit the data? If we use memory mode, we can avoid committing the blocks.
+1
There was a problem hiding this comment.
Why do we need commit the data? If we use memory mode, we can avoid committing the blocks.
But we have an option not to use memory, or we should disable this option in non-unit test scenarios.
There was a problem hiding this comment.
Think twice. It may not influence our correctness. Because we don't use committed bitmap to judge correctness.
3 participants
What changes were proposed in this pull request?
Replace Roaring64NavigableMap with AtomicLong for checking commit result
Why are the changes needed?
Fix: #2740
To save memory.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
CI