Skip to content

Commit 6acfd53

Browse files
authored
fix(spark): Increment segmentIndex when skipping segment due to crc check failure (#2746)
### What changes were proposed in this pull request? When segment is skipped, we should increment the segmentIndex; otherwise, it may lead to data corruption. ### Why are the changes needed? fix bug ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? added test case
1 parent eb69200 commit 6acfd53

2 files changed

Lines changed: 32 additions & 5 deletions

File tree

client/src/main/java/org/apache/uniffle/client/impl/ShuffleReadClientImpl.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,11 @@ public ShuffleBlock readShuffleBlockData() {
297297
if (shuffleServerInfoList.size() > 1) {
298298
LOG.warn(errMsg);
299299
clientReadHandler.updateConsumedBlockInfo(bs, true);
300+
if (decompressionWorker != null) {
301+
decompressionWorker.get(batchIndex - 1, segmentIndex++);
302+
} else {
303+
segmentIndex += 1;
304+
}
300305
continue;
301306
} else {
302307
throw new RssFetchFailedException(errMsg);

client/src/test/java/org/apache/uniffle/client/impl/ShuffleReadClientImplTest.java

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
package org.apache.uniffle.client.impl;
1919

2020
import java.nio.ByteBuffer;
21+
import java.util.LinkedHashMap;
2122
import java.util.List;
2223
import java.util.Map;
2324
import java.util.Random;
@@ -38,7 +39,6 @@
3839

3940
import org.apache.uniffle.client.TestUtils;
4041
import org.apache.uniffle.client.factory.ShuffleClientFactory;
41-
import org.apache.uniffle.client.response.ShuffleBlock;
4242
import org.apache.uniffle.common.ClientType;
4343
import org.apache.uniffle.common.ShufflePartitionedBlock;
4444
import org.apache.uniffle.common.ShuffleServerInfo;
@@ -59,6 +59,7 @@
5959
import static org.junit.jupiter.api.Assertions.assertTrue;
6060
import static org.junit.jupiter.api.Assertions.fail;
6161
import static org.mockito.ArgumentMatchers.any;
62+
import static org.mockito.ArgumentMatchers.anyInt;
6263

6364
public class ShuffleReadClientImplTest extends HadoopTestBase {
6465

@@ -371,11 +372,14 @@ public void readTest8(Supplier<ShuffleClientFactory.ReadClientBuilder> builderSu
371372
String basePath = uniq(HDFS_URI + "clientReadTest8");
372373
HadoopShuffleWriteHandler writeHandler =
373374
new HadoopShuffleWriteHandler("appId", 0, 0, 1, basePath, ssi1.getId(), conf);
375+
HadoopShuffleWriteHandler writeHandler2 =
376+
new HadoopShuffleWriteHandler("appId", 0, 0, 1, basePath, ssi2.getId(), conf);
374377

375-
Map<Long, byte[]> expectedData = Maps.newHashMap();
378+
LinkedHashMap<Long, byte[]> expectedData = Maps.newLinkedHashMap();
376379
Roaring64NavigableMap blockIdBitmap = Roaring64NavigableMap.bitmapOf();
377380
Roaring64NavigableMap taskIdBitmap = Roaring64NavigableMap.bitmapOf(0);
378381
writeTestData(writeHandler, 2, 30, 0, 0, expectedData, blockIdBitmap);
382+
writeTestData(writeHandler2, 2, 30, 0, 0, expectedData, blockIdBitmap);
379383
ShuffleReadClientImpl readClient =
380384
builderSupplier
381385
.get()
@@ -396,8 +400,19 @@ public void readTest8(Supplier<ShuffleClientFactory.ReadClientBuilder> builderSu
396400
.shuffleServerInfoList(Lists.newArrayList(ssi1, ssi2))
397401
.build();
398402
// crc32 is incorrect
403+
AtomicInteger readCount = new AtomicInteger(0);
399404
try (MockedStatic<ChecksumUtils> checksumUtilsMock = Mockito.mockStatic(ChecksumUtils.class)) {
400-
checksumUtilsMock.when(() -> ChecksumUtils.getCrc32((ByteBuffer) any())).thenReturn(-1L);
405+
checksumUtilsMock
406+
.when(() -> ChecksumUtils.getCrc32(any(ByteBuffer.class), anyInt(), anyInt()))
407+
.then(
408+
invocation -> {
409+
// crc check fails for readClient1 and frist block of readClient2
410+
if (readCount.getAndIncrement() < 2) {
411+
return -1;
412+
} else {
413+
return invocation.callRealMethod();
414+
}
415+
});
401416
try {
402417
ByteBuffer bb = readClient.readShuffleBlockData().getByteBuffer();
403418
while (bb != null) {
@@ -408,8 +423,15 @@ public void readTest8(Supplier<ShuffleClientFactory.ReadClientBuilder> builderSu
408423
assertTrue(e.getMessage().startsWith("Unexpected crc value"), e.getMessage());
409424
}
410425

411-
ShuffleBlock block = readClient2.readShuffleBlockData();
412-
assertNull(block);
426+
// the frist block has been skipped due to crc check failure
427+
Long firstKey = expectedData.keySet().iterator().next();
428+
expectedData.remove(firstKey);
429+
TestUtils.validateResult(readClient2, expectedData);
430+
try {
431+
readClient2.checkProcessedBlockIds();
432+
} catch (Exception e) {
433+
assertTrue(e.getMessage().contains("expected 4 blocks, actual 3 blocks"), e.getMessage());
434+
}
413435
}
414436
readClient.close();
415437
readClient2.close();

0 commit comments

Comments
 (0)