Skip to content

Commit 157c1bd

Browse files
KaiSernLimclaude
andcommitted
Add unit tests for Global RT DIV state methods to fix diff coverage
Add tests covering the new branches introduced for Global RT DIV state storage and retrieval: - RocksDBStorageEngineTest: tests for putGlobalRtDivState, getGlobalRtDivState, and clearGlobalRtDivState happy paths and illegal argument branches (negative partition id, metadata partition id) - LeaderFollowerStoreIngestionTaskTest: tests for putGlobalRtDivStateInMetadata (invalid key prefix exception and valid delegation) and readGlobalRtDivState (metadata-present early return, metadata-absent fallthrough, and non-prefix key skip) Diff branch coverage for da-vinci-client goes from 11% to 67.65%, exceeding the 45% minimum threshold. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 939079a commit 157c1bd

2 files changed

Lines changed: 151 additions & 0 deletions

File tree

clients/da-vinci-client/src/test/java/com/linkedin/davinci/kafka/consumer/LeaderFollowerStoreIngestionTaskTest.java

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import com.linkedin.davinci.stats.ingestion.heartbeat.HeartbeatMonitoringService;
3838
import com.linkedin.davinci.storage.StorageMetadataService;
3939
import com.linkedin.davinci.storage.StorageService;
40+
import com.linkedin.davinci.storage.chunking.ChunkedValueManifestContainer;
4041
import com.linkedin.davinci.store.DelegatingStorageEngine;
4142
import com.linkedin.davinci.store.view.MaterializedViewWriter;
4243
import com.linkedin.davinci.store.view.VeniceViewWriter;
@@ -45,6 +46,7 @@
4546
import com.linkedin.davinci.validation.PartitionTracker;
4647
import com.linkedin.venice.compression.CompressionStrategy;
4748
import com.linkedin.venice.compression.VeniceCompressor;
49+
import com.linkedin.venice.exceptions.VeniceException;
4850
import com.linkedin.venice.exceptions.VeniceTimeoutException;
4951
import com.linkedin.venice.kafka.protocol.ControlMessage;
5052
import com.linkedin.venice.kafka.protocol.Delete;
@@ -96,6 +98,7 @@
9698
import com.linkedin.venice.writer.VeniceWriter;
9799
import it.unimi.dsi.fastutil.objects.Object2IntMaps;
98100
import java.io.IOException;
101+
import java.lang.reflect.Field;
99102
import java.nio.ByteBuffer;
100103
import java.util.Collections;
101104
import java.util.HashMap;
@@ -1397,4 +1400,97 @@ public void testDolStampIsDolComplete() {
13971400
dolStampIncomplete.setDolConsumed(true);
13981401
assertFalse(dolStampIncomplete.isDolComplete());
13991402
}
1403+
1404+
/**
1405+
* Tests that {@link StoreIngestionTask#putGlobalRtDivStateInMetadata} throws a VeniceException
1406+
* when the key does not start with the expected prefix, and delegates to storageMetadataService
1407+
* when the key is valid.
1408+
*/
1409+
@Test
1410+
public void testPutGlobalRtDivStateInMetadata() throws Exception {
1411+
LeaderFollowerStoreIngestionTask ingestionTask = mock(LeaderFollowerStoreIngestionTask.class);
1412+
doCallRealMethod().when(ingestionTask).putGlobalRtDivStateInMetadata(anyInt(), any(), any());
1413+
1414+
Put put = new Put();
1415+
put.putValue = ByteBuffer.wrap("test-value".getBytes());
1416+
1417+
// Invalid key (missing prefix) should throw VeniceException immediately, before any field access
1418+
byte[] invalidKey = "INVALID_KEY.localhost:9092".getBytes();
1419+
Assert.assertThrows(VeniceException.class, () -> ingestionTask.putGlobalRtDivStateInMetadata(0, invalidKey, put));
1420+
1421+
// Valid key should call storageMetadataService.putGlobalRtDivState
1422+
StorageMetadataService mockSms = mock(StorageMetadataService.class);
1423+
String versionTopic = "testStore_v1";
1424+
injectField(ingestionTask, StoreIngestionTask.class, "storageMetadataService", mockSms);
1425+
injectField(ingestionTask, StoreIngestionTask.class, "kafkaVersionTopic", versionTopic);
1426+
1427+
String brokerUrl = "localhost:9092";
1428+
byte[] validKey = (StoreIngestionTask.GLOBAL_RT_DIV_KEY_PREFIX + brokerUrl).getBytes();
1429+
ingestionTask.putGlobalRtDivStateInMetadata(0, validKey, put);
1430+
1431+
ArgumentCaptor<byte[]> valueCaptor = ArgumentCaptor.forClass(byte[].class);
1432+
verify(mockSms, times(1)).putGlobalRtDivState(eq(versionTopic), eq(0), eq(brokerUrl), valueCaptor.capture());
1433+
}
1434+
1435+
/**
1436+
* Tests the new metadata-first lookup in {@link LeaderFollowerStoreIngestionTask#readGlobalRtDivState}:
1437+
* - When metadata storage has a value, it is returned without hitting legacy storage.
1438+
* - When metadata storage is empty, the method falls through to the legacy path.
1439+
* - When the key does not start with the GLOBAL_RT_DIV_KEY_PREFIX, the metadata lookup is skipped.
1440+
*/
1441+
@Test
1442+
public void testReadGlobalRtDivStateMetadataPath() throws Exception {
1443+
LeaderFollowerStoreIngestionTask ingestionTask = mock(LeaderFollowerStoreIngestionTask.class);
1444+
doCallRealMethod().when(ingestionTask)
1445+
.readGlobalRtDivState(any(), anyInt(), any(), any(ChunkedValueManifestContainer.class));
1446+
1447+
StorageMetadataService mockSms = mock(StorageMetadataService.class);
1448+
String versionTopic = "testStore_v1";
1449+
InternalAvroSpecificSerializer<GlobalRtDivState> serializer =
1450+
AvroProtocolDefinition.GLOBAL_RT_DIV_STATE.getSerializer();
1451+
1452+
injectField(ingestionTask, StoreIngestionTask.class, "storageMetadataService", mockSms);
1453+
injectField(ingestionTask, StoreIngestionTask.class, "kafkaVersionTopic", versionTopic);
1454+
injectField(ingestionTask, LeaderFollowerStoreIngestionTask.class, "globalRtDivStateSerializer", serializer);
1455+
1456+
String brokerUrl = "localhost:9092";
1457+
byte[] keyBytes = (StoreIngestionTask.GLOBAL_RT_DIV_KEY_PREFIX + brokerUrl).getBytes();
1458+
PubSubTopicPartition topicPartition = mock(PubSubTopicPartition.class);
1459+
doReturn(0).when(topicPartition).getPartitionNumber();
1460+
ChunkedValueManifestContainer manifestContainer = new ChunkedValueManifestContainer();
1461+
1462+
// Serialize a real GlobalRtDivState for the round-trip test
1463+
GlobalRtDivState expectedState =
1464+
new GlobalRtDivState(brokerUrl, Collections.emptyMap(), InMemoryPubSubPosition.of(5).toWireFormatBuffer());
1465+
byte[] serializedState = serializer.serialize(null, expectedState);
1466+
1467+
// Case 1: metadata present → returns deserialized state without touching legacy storage
1468+
doReturn(Optional.of(serializedState)).when(mockSms).getGlobalRtDivState(versionTopic, 0, brokerUrl);
1469+
GlobalRtDivState result =
1470+
ingestionTask.readGlobalRtDivState(keyBytes, GLOBAL_RT_DIV_VERSION, topicPartition, manifestContainer);
1471+
assertNotNull(result);
1472+
assertEquals(result.srcUrl.toString(), brokerUrl);
1473+
1474+
// Case 2: metadata absent → falls through to legacy; legacy returns null (compressor field is null on mock),
1475+
// so the method returns null
1476+
doReturn(Optional.empty()).when(mockSms).getGlobalRtDivState(versionTopic, 0, brokerUrl);
1477+
GlobalRtDivState fallThroughResult =
1478+
ingestionTask.readGlobalRtDivState(keyBytes, GLOBAL_RT_DIV_VERSION, topicPartition, manifestContainer);
1479+
Assert.assertNull(fallThroughResult);
1480+
1481+
// Case 3: key does not start with the prefix → metadata lookup skipped, goes straight to legacy (returns null)
1482+
byte[] nonPrefixKey = "REGULAR_KEY.localhost:9092".getBytes();
1483+
GlobalRtDivState nonPrefixResult =
1484+
ingestionTask.readGlobalRtDivState(nonPrefixKey, GLOBAL_RT_DIV_VERSION, topicPartition, manifestContainer);
1485+
Assert.assertNull(nonPrefixResult);
1486+
// storageMetadataService.getGlobalRtDivState should NOT have been called for the non-prefix key
1487+
verify(mockSms, times(0)).getGlobalRtDivState(versionTopic, 0, "REGULAR_KEY.localhost:9092".substring(0));
1488+
}
1489+
1490+
private static void injectField(Object target, Class<?> declaringClass, String fieldName, Object value)
1491+
throws Exception {
1492+
Field field = declaringClass.getDeclaredField(fieldName);
1493+
field.setAccessible(true);
1494+
field.set(target, value);
1495+
}
14001496
}

clients/da-vinci-client/src/test/java/com/linkedin/davinci/store/rocksdb/RocksDBStorageEngineTest.java

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,4 +288,59 @@ public void testHasConflictPersistedStoreEngineConfig() {
288288
Assert.assertTrue(result);
289289

290290
}
291+
292+
@Test
293+
public void testGetAndPutGlobalRtDivState() {
294+
RocksDBStorageEngine rocksDBStorageEngine = (RocksDBStorageEngine) getTestStoreEngine();
295+
int partitionId = PARTITION_ID;
296+
String brokerUrl = "localhost:9092";
297+
byte[] valueBytes = "test-global-rt-div-value".getBytes();
298+
299+
// Put and get round-trip
300+
rocksDBStorageEngine.putGlobalRtDivState(partitionId, brokerUrl, valueBytes);
301+
Assert.assertTrue(rocksDBStorageEngine.getGlobalRtDivState(partitionId, brokerUrl).isPresent());
302+
Assert.assertEquals(rocksDBStorageEngine.getGlobalRtDivState(partitionId, brokerUrl).get(), valueBytes);
303+
304+
// Clear and verify absent
305+
rocksDBStorageEngine.clearGlobalRtDivState(partitionId, brokerUrl);
306+
Assert.assertFalse(rocksDBStorageEngine.getGlobalRtDivState(partitionId, brokerUrl).isPresent());
307+
308+
// Different brokerUrls are stored independently
309+
byte[] value1 = "value1".getBytes();
310+
byte[] value2 = "value2".getBytes();
311+
rocksDBStorageEngine.putGlobalRtDivState(partitionId, "broker1:9092", value1);
312+
rocksDBStorageEngine.putGlobalRtDivState(partitionId, "broker2:9092", value2);
313+
Assert.assertEquals(rocksDBStorageEngine.getGlobalRtDivState(partitionId, "broker1:9092").get(), value1);
314+
Assert.assertEquals(rocksDBStorageEngine.getGlobalRtDivState(partitionId, "broker2:9092").get(), value2);
315+
316+
// Clean up
317+
rocksDBStorageEngine.clearGlobalRtDivState(partitionId, "broker1:9092");
318+
rocksDBStorageEngine.clearGlobalRtDivState(partitionId, "broker2:9092");
319+
}
320+
321+
@Test
322+
public void testIllegalPartitionIdInGlobalRtDivState() {
323+
RocksDBStorageEngine rocksDBStorageEngine = (RocksDBStorageEngine) getTestStoreEngine();
324+
String brokerUrl = "localhost:9092";
325+
byte[] valueBytes = "value".getBytes();
326+
327+
// Negative partition id should throw
328+
Assert.assertThrows(
329+
IllegalArgumentException.class,
330+
() -> rocksDBStorageEngine.putGlobalRtDivState(-1, brokerUrl, valueBytes));
331+
Assert.assertThrows(IllegalArgumentException.class, () -> rocksDBStorageEngine.getGlobalRtDivState(-1, brokerUrl));
332+
Assert
333+
.assertThrows(IllegalArgumentException.class, () -> rocksDBStorageEngine.clearGlobalRtDivState(-1, brokerUrl));
334+
335+
// Metadata partition id should throw
336+
Assert.assertThrows(
337+
IllegalArgumentException.class,
338+
() -> rocksDBStorageEngine.putGlobalRtDivState(METADATA_PARTITION_ID, brokerUrl, valueBytes));
339+
Assert.assertThrows(
340+
IllegalArgumentException.class,
341+
() -> rocksDBStorageEngine.getGlobalRtDivState(METADATA_PARTITION_ID, brokerUrl));
342+
Assert.assertThrows(
343+
IllegalArgumentException.class,
344+
() -> rocksDBStorageEngine.clearGlobalRtDivState(METADATA_PARTITION_ID, brokerUrl));
345+
}
291346
}

0 commit comments

Comments
 (0)