[Test] Assume unknown blockID in LocalFileHandlerTestBase #478
Conversation
Codecov Report
@@ Coverage Diff @@
## master #478 +/- ##
============================================
- Coverage 58.78% 58.77% -0.01%
- Complexity 1704 1705 +1
============================================
Files 206 206
Lines 11471 11469 -2
Branches 1024 1023 -1
============================================
- Hits 6743 6741 -2
Misses 4317 4317
Partials 411 411
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Did a quick overview of the related code, I believe the better fix should be refactor In a real environment, the blockId could be random and generated concurrently. also cc @zuston |
I prefer to this solution |
…stDataInconsistent (apache#473)" This reverts commit 7f89d6f.
Not all blocks in |
What about passing the correct wroteBlockList |
kaijchen
force-pushed
the
refactor-LocalFileHandlerTestBase
branch
from
a8d2726
to
620464f
Compare
kaijchen
changed the title
| ShuffleWriteHandler writeHandler, | ||
| int num, int length, | ||
| Map<Long, byte[]> expectedData, | ||
| Set<Long> expectedBlockIds) throws Exception { | ||
| List<ShufflePartitionedBlock> blocks = Lists.newArrayList(); | ||
| List<Long> blockIds = Lists.newArrayList(); |
There was a problem hiding this comment.
Why not use Set<Long> expectedBlockIds directly?
There was a problem hiding this comment.
Can you give an example how to use expectedBlockIds in calcSegmentBytes?
I think we should preserve order and exclude the last block.
There was a problem hiding this comment.
Thanks, I got it when you mean TreeSet. However, I'm wondering if we should keep the current writeTestData() signature.
There was a problem hiding this comment.
Let's use LinkedHashSet for expectedBlockIds?
The signature could retain or just changed to LinkedHashSet<Long>. This is the test method, we can change it at will.
| @@ -80,7 +78,7 @@ public void testDataInconsistent() throws Exception { | |||
| int readBufferSize = 13; | |||
| int bytesPerSegment = ((readBufferSize / blockSize) + 1) * blockSize; | |||
| List<byte[]> segments = LocalFileHandlerTestBase.calcSegmentBytes(expectedData, | |||
| bytesPerSegment, actualWriteDataBlock); | |||
| bytesPerSegment, blockIds.subList(0, actualWriteDataBlock)); | |||
There was a problem hiding this comment.
@zuston because we need to get subList here.
There was a problem hiding this comment.
The size of expectedBlockIds is expected in this method of calcSegmentBytes invoking. Right?
|
Thanks @zuston and @advancedxy for the review. |
| Map<Long, byte[]> expectedData, Set<Long> expectedBlockIds) throws Exception { | ||
| handler.write(blocks); | ||
| blocks.forEach(block -> expectedBlockIds.add(block.getBlockId())); | ||
| blocks.forEach(block -> expectedData.put(block.getBlockId(), block.getData())); |
There was a problem hiding this comment.
Why not do above two operations in one foreach ?
There was a problem hiding this comment.
There is no significant peformance difference. And I prefer do one thing at a time for clarity.
|
Thanks @advancedxy and @zuston for the review. |
What changes were proposed in this pull request?
Split
LocalFileHandlerTestBase#writeTestData()intogenerateBlocks()andwriteTestData().Pass
List<Long> blockIdsintoLocalFileHandlerTestBase#calcSegmentBytes().Why are the changes needed?
Followup #473.
To make the expected result corresponding to the state of
LocalFileHandlerTestBase#ATOMIC_LONG.Does this PR introduce any user-facing change?
No.
How was this patch tested?
CI.