Skip to content

Commit 9ac8c24

Browse files
KaiSernLimclaude
andauthored
[da-vinci] Global RT DIV: Per-Partition Consumed Bytes Counter (linkedin#2739)
consumedBytesSinceLastSync was a Map<String, Long> on StoreIngestionTask, shared across all partitions of the same version. This meant the sync cadence for shouldSendGlobalRtDiv and shouldSyncOffsetFromSnapshot was driven by aggregate bytes across all partitions rather than per-partition progress. Fix: Remove the SIT-level field entirely. Add consumedBytesSinceLastGlobalRtDivSync: Map<String, Long> to PartitionConsumptionState with get/add/reset methods, keyed by VT name (for local VT tracking) or broker URL (for RT tracking). --------- Co-authored-by: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
1 parent 52134af commit 9ac8c24

5 files changed

Lines changed: 58 additions & 39 deletions

File tree

clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/LeaderFollowerStoreIngestionTask.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3174,7 +3174,7 @@ void syncOffsetFromSnapshotIfNeeded(DefaultPubSubMessage record, PubSubTopicPart
31743174
storeBufferService.execSyncOffsetFromSnapshotAsync(topicPartition, vtDiv, lastFuture, this);
31753175
// Reset consumer-side VT bytes so the size-based condition in shouldSyncOffsetFromSnapshot does not keep
31763176
// firing for every subsequent record.
3177-
getConsumedBytesSinceLastSync().put(getVersionTopic().getName(), 0L);
3177+
pcs.resetConsumedBytesSinceLastGlobalRtDivSync(getVersionTopic().getName());
31783178

31793179
// TODO: remove. this is a temporary log for debugging while the feature is in its infancy
31803180
LOGGER.info(
@@ -3210,7 +3210,7 @@ boolean shouldSyncOffsetFromSnapshot(DefaultPubSubMessage consumerRecord, Partit
32103210

32113211
// must be greater than the interval in shouldSendGlobalRtDiv() to not interfere
32123212
final long syncBytesInterval = getSyncBytesInterval(pcs); // size-based sync condition
3213-
long vtConsumedBytesSinceLastSync = getConsumedBytesSinceLastSync().getOrDefault(getVersionTopic().getName(), 0L);
3213+
long vtConsumedBytesSinceLastSync = pcs.getConsumedBytesSinceLastGlobalRtDivSync(getVersionTopic().getName());
32143214
return syncBytesInterval > 0 && (vtConsumedBytesSinceLastSync >= 2 * syncBytesInterval);
32153215
}
32163216

@@ -3985,7 +3985,7 @@ void sendGlobalRtDivMessage(
39853985
null,
39863986
true);
39873987

3988-
consumedBytesSinceLastSync.put(brokerUrl, 0L); // reset the timer for the next sync, since RT DIV was just synced
3988+
pcs.resetConsumedBytesSinceLastGlobalRtDivSync(brokerUrl);
39893989
}
39903990

39913991
private byte[] createGlobalRtDivValueBytes(

clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/PartitionConsumptionState.java

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,12 @@ enum LatchStatus {
162162
*/
163163
private long processedRecordSizeSinceLastSync;
164164

165+
/**
166+
* Tracks bytes consumed per source key (VT name or RT broker URL) since the last Global RT DIV sync.
167+
* Stored per-partition so that each partition's sync cadence is independent.
168+
*/
169+
private final Map<String, Long> consumedBytesSinceLastGlobalRtDivSync = new VeniceConcurrentHashMap<>();
170+
165171
/** Minimum lgK supported by DataSketches HllSketch (mirrors package-private HllUtil.MIN_LOG_K). */
166172
static final int HLL_MIN_LOG_K = 4;
167173
/** Maximum lgK supported by DataSketches HllSketch (mirrors package-private HllUtil.MAX_LOG_K). */
@@ -718,6 +724,21 @@ public void resetProcessedRecordSizeSinceLastSync() {
718724
this.processedRecordSizeSinceLastSync = 0;
719725
}
720726

727+
public long getConsumedBytesSinceLastGlobalRtDivSync(String key) {
728+
return consumedBytesSinceLastGlobalRtDivSync.getOrDefault(key, 0L);
729+
}
730+
731+
public void addConsumedBytesSinceLastGlobalRtDivSync(String key, long bytes) {
732+
if (bytes <= 0) {
733+
return;
734+
}
735+
consumedBytesSinceLastGlobalRtDivSync.merge(key, bytes, Long::sum);
736+
}
737+
738+
public void resetConsumedBytesSinceLastGlobalRtDivSync(String key) {
739+
consumedBytesSinceLastGlobalRtDivSync.put(key, 0L);
740+
}
741+
721742
public void setLeaderFollowerState(LeaderFollowerStateType state) {
722743
this.leaderFollowerState = state;
723744
}

clients/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java

Lines changed: 8 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -311,9 +311,6 @@ public abstract class StoreIngestionTask implements Runnable, Closeable {
311311
* to local VT. This will also be used to send DIV snapshots to the drainer to persist the VT + RT DIV on-disk.
312312
*/
313313
protected final DataIntegrityValidator consumerDiv;
314-
/** Map of (RT broker URL | VT name) to the total bytes consumed by ConsumptionTask since the last Global RT DIV sync */
315-
// TODO: clear it out when the sync is done
316-
protected final VeniceConcurrentHashMap<String, Long> consumedBytesSinceLastSync;
317314
protected final HostLevelIngestionStats hostLevelIngestionStats;
318315
protected final AggVersionedDIVStats versionedDIVStats;
319316
protected final AggVersionedIngestionStats versionedIngestionStats;
@@ -535,7 +532,6 @@ public StoreIngestionTask(
535532
pubSubContext.getPubSubPositionDeserializer(),
536533
DISABLED,
537534
producerStateMaxAgeMs);
538-
this.consumedBytesSinceLastSync = new VeniceConcurrentHashMap<>();
539535
this.ingestionTaskName = String.format(CONSUMER_TASK_ID_FORMAT, kafkaVersionTopic);
540536
this.readOnlyForBatchOnlyStoreEnabled = storeVersionConfig.isReadOnlyForBatchOnlyStoreEnabled();
541537
this.hostLevelIngestionStats = builder.getIngestionStats().getStoreStats(storeName);
@@ -1494,7 +1490,7 @@ protected void produceToStoreBufferServiceOrKafka(
14941490
PubSubTopic topic = topicPartition.getPubSubTopic();
14951491
if (isGlobalRtDivEnabled() && (versionTopic.equals(topic) || topic.isRealTime())) {
14961492
String consumedBytesKey = versionTopic.equals(topic) ? versionTopic.getName() : kafkaUrl;
1497-
consumedBytesSinceLastSync.compute(consumedBytesKey, (k, v) -> (v == null) ? recordSize : v + recordSize);
1493+
partitionConsumptionState.addConsumedBytesSinceLastGlobalRtDivSync(consumedBytesKey, recordSize);
14981494
}
14991495
}
15001496

@@ -1582,7 +1578,7 @@ protected void produceToStoreBufferServiceOrKafkaInBatch(
15821578
linkBackManifestFromTransientRecord(processedRecord, partitionConsumptionState);
15831579
}
15841580

1585-
totalBytesRead += handleSingleMessage(
1581+
int recordSize = handleSingleMessage(
15861582
processedRecord,
15871583
topicPartition,
15881584
partitionConsumptionState,
@@ -1591,6 +1587,11 @@ protected void produceToStoreBufferServiceOrKafkaInBatch(
15911587
beforeProcessingPerRecordTimestampNs,
15921588
beforeProcessingBatchRecordsTimestampMs,
15931589
elapsedTimeForPuttingIntoQueue);
1590+
totalBytesRead += recordSize;
1591+
// Batch path only handles RT messages (guaranteed by isAllMessagesFromRTTopic), so key by kafkaUrl.
1592+
if (isGlobalRtDivEnabled()) {
1593+
partitionConsumptionState.addConsumedBytesSinceLastGlobalRtDivSync(kafkaUrl, recordSize);
1594+
}
15941595

15951596
// Only track keys that were actually produced (not ignored by DCR).
15961597
// Ignored records don't call setChunkingInfo, so the transient record's
@@ -3372,7 +3373,7 @@ boolean shouldSendGlobalRtDiv(DefaultPubSubMessage record, PartitionConsumptionS
33723373
return false;
33733374
}
33743375
final long syncBytesInterval = getSyncBytesInterval(pcs);
3375-
return syncBytesInterval > 0 && (getConsumedBytesSinceLastSync().getOrDefault(brokerUrl, 0L) >= syncBytesInterval);
3376+
return syncBytesInterval > 0 && (pcs.getConsumedBytesSinceLastGlobalRtDivSync(brokerUrl) >= syncBytesInterval);
33763377
}
33773378

33783379
abstract void syncOffsetFromSnapshotIfNeeded(DefaultPubSubMessage record, PubSubTopicPartition topicPartition);
@@ -5672,10 +5673,6 @@ boolean isDaVinciClient() {
56725673
return isDaVinciClient;
56735674
}
56745675

5675-
VeniceConcurrentHashMap<String, Long> getConsumedBytesSinceLastSync() {
5676-
return consumedBytesSinceLastSync; // mainly for unit test mocks
5677-
}
5678-
56795676
boolean isGlobalRtDivEnabled() {
56805677
return isGlobalRtDivEnabled;
56815678
}

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -859,8 +859,6 @@ public void testShouldSyncOffsetFromSnapshot() throws InterruptedException {
859859
// Stub early so size-based branch can call getVersionTopic().getName()
860860
PubSubTopic versionTopic = TOPIC_REPOSITORY.getTopic("test-topic_v1");
861861
doReturn(versionTopic).when(mockIngestionTask).getVersionTopic();
862-
VeniceConcurrentHashMap<String, Long> consumedBytesSinceLastSync = new VeniceConcurrentHashMap<>();
863-
doReturn(consumedBytesSinceLastSync).when(mockIngestionTask).getConsumedBytesSinceLastSync();
864862

865863
// Set up Global RT DIV message
866864
final DefaultPubSubMessage globalRtDivMessage = getMockMessage(1).getMessage();
@@ -913,15 +911,18 @@ public void testShouldSyncOffsetFromSnapshot() throws InterruptedException {
913911
doReturn(false).when(regularMockKey).isControlMessage();
914912

915913
// Test case 1: When VT consumed bytes since last sync is less than 2*syncBytesInterval
916-
consumedBytesSinceLastSync.put(versionTopic.getName(), 1500L);
914+
doReturn(1500L).when(mockPartitionConsumptionState)
915+
.getConsumedBytesSinceLastGlobalRtDivSync(versionTopic.getName());
917916
assertFalse(mockIngestionTask.shouldSyncOffsetFromSnapshot(regularMessage, mockPartitionConsumptionState));
918917

919918
// Test case 2: When VT consumed bytes since last sync is equal to 2*syncBytesInterval
920-
consumedBytesSinceLastSync.put(versionTopic.getName(), 2000L);
919+
doReturn(2000L).when(mockPartitionConsumptionState)
920+
.getConsumedBytesSinceLastGlobalRtDivSync(versionTopic.getName());
921921
assertTrue(mockIngestionTask.shouldSyncOffsetFromSnapshot(regularMessage, mockPartitionConsumptionState));
922922

923923
// Test case 3: When VT consumed bytes since last sync is greater than 2*syncBytesInterval
924-
consumedBytesSinceLastSync.put(versionTopic.getName(), 2500L);
924+
doReturn(2500L).when(mockPartitionConsumptionState)
925+
.getConsumedBytesSinceLastGlobalRtDivSync(versionTopic.getName());
925926
assertTrue(mockIngestionTask.shouldSyncOffsetFromSnapshot(regularMessage, mockPartitionConsumptionState));
926927

927928
// Test case 4: When syncBytesInterval is 0 (disabled)
@@ -973,8 +974,6 @@ public void testSyncOffsetFromSnapshotIfNeededSkipsWhenLcvpIsEarliest() throws I
973974
doReturn(ApacheKafkaOffsetPosition.of(10L)).when(snapshotReady).getLatestConsumedVtPosition();
974975
doReturn(Collections.singletonMap("producerGuid", new Object())).when(snapshotReady).getPartitionStates(any());
975976
doReturn(snapshotReady).when(mockConsumerDiv).cloneVtProducerStates(anyInt(), anyBoolean(), anyLong());
976-
VeniceConcurrentHashMap<String, Long> consumedBytes = new VeniceConcurrentHashMap<>();
977-
doReturn(consumedBytes).when(leaderFollowerStoreIngestionTask).getConsumedBytesSinceLastSync();
978977

979978
leaderFollowerStoreIngestionTask.syncOffsetFromSnapshotIfNeeded(mockRecord, versionTopicPartition);
980979
verify(mockStoreBufferService, times(1)).execSyncOffsetFromSnapshotAsync(any(), any(), any(), any());

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

Lines changed: 20 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@
6060
import static org.mockito.Mockito.atLeast;
6161
import static org.mockito.Mockito.atLeastOnce;
6262
import static org.mockito.Mockito.atMost;
63+
import static org.mockito.Mockito.clearInvocations;
6364
import static org.mockito.Mockito.doAnswer;
6465
import static org.mockito.Mockito.doCallRealMethod;
6566
import static org.mockito.Mockito.doNothing;
@@ -6101,14 +6102,16 @@ public void testShouldSendGlobalRtDiv(boolean isGlobalRtDivEnabled) {
61016102
doReturn(key).when(message).getKey();
61026103
PartitionConsumptionState pcs = mock(PartitionConsumptionState.class);
61036104
doReturn(100L).when(pcs).getProcessedRecordSizeSinceLastSync(); // just needs to be greater than syncBytesInterval
6104-
VeniceConcurrentHashMap<String, Long> lastProcessedMap = new VeniceConcurrentHashMap<>();
6105-
doReturn(lastProcessedMap).when(storeIngestionTask).getConsumedBytesSinceLastSync();
6105+
// Default: 0 bytes consumed, so shouldSendGlobalRtDiv returns false regardless of host
6106+
doReturn(0L).when(pcs).getConsumedBytesSinceLastGlobalRtDivSync(any());
61066107

6107-
// Two sanity tests: empty map should not cause divide by zero, and host not present in map should return false
6108+
// Two sanity tests: 0 bytes should not trigger sync, and an untracked host key returning 0 bytes should return
6109+
// false
61086110
storeIngestionTask.shouldSendGlobalRtDiv(message, pcs, brokerUrl);
61096111
assertFalse(storeIngestionTask.shouldSendGlobalRtDiv(message, pcs, "fakehost:5678"));
61106112

6111-
lastProcessedMap.put(brokerUrl, 100L); // just needs to be greater than syncBytesInterval
6113+
doReturn(100L).when(pcs).getConsumedBytesSinceLastGlobalRtDivSync(brokerUrl); // just needs to be >=
6114+
// syncBytesInterval
61126115
boolean shouldSendGlobalRtDiv = storeIngestionTask.shouldSendGlobalRtDiv(message, pcs, brokerUrl);
61136116
boolean shouldSyncOffset = storeIngestionTask.shouldSyncOffset(pcs, message, null);
61146117

@@ -6121,12 +6124,12 @@ public void testShouldSendGlobalRtDiv(boolean isGlobalRtDivEnabled) {
61216124
}
61226125

61236126
/**
6124-
* Verifies that {@link StoreIngestionTask#produceToStoreBufferServiceOrKafka} populates
6125-
* {@link StoreIngestionTask#consumedBytesSinceLastSync} only for the local VT (keyed by VT name)
6126-
* and RT topics (keyed by broker URL). Remote VTs must be excluded from the map entirely.
6127+
* Verifies that {@link StoreIngestionTask#produceToStoreBufferServiceOrKafka} calls
6128+
* {@link PartitionConsumptionState#addConsumedBytesSinceLastGlobalRtDivSync} only for the local VT
6129+
* (keyed by VT name) and RT topics (keyed by broker URL). Remote VTs must be excluded entirely.
61276130
*/
61286131
@Test
6129-
public void testConsumedBytesSinceLastSyncTracking() throws Exception {
6132+
public void testConsumedBytesSinceLastGlobalRtDivSyncTracking() throws Exception {
61306133
String storeName = "test-store";
61316134
PubSubTopic localVt = pubSubTopicRepository.getTopic(Version.composeKafkaTopic(storeName, 1));
61326135
PubSubTopic rtTopic = pubSubTopicRepository.getTopic(storeName + "_rt");
@@ -6143,17 +6146,16 @@ public void testConsumedBytesSinceLastSyncTracking() throws Exception {
61436146
doReturn(StoreIngestionTask.DelegateConsumerRecordResult.SKIPPED_MESSAGE).when(sit)
61446147
.delegateConsumerRecord(any(), anyInt(), any(), anyInt(), anyLong(), anyLong());
61456148

6146-
VeniceConcurrentHashMap<String, Long> consumedBytesMap = new VeniceConcurrentHashMap<>();
6149+
PartitionConsumptionState pcs = mock(PartitionConsumptionState.class);
61476150
VeniceConcurrentHashMap<Integer, PartitionConsumptionState> pcsMap = new VeniceConcurrentHashMap<>();
6148-
pcsMap.put(partition, mock(PartitionConsumptionState.class));
6151+
pcsMap.put(partition, pcs);
61496152

61506153
for (String fieldName: new String[] { "isActiveActiveReplicationEnabled", "isWriteComputationEnabled" }) {
61516154
Field f = StoreIngestionTask.class.getDeclaredField(fieldName);
61526155
f.setAccessible(true);
61536156
f.set(sit, false);
61546157
}
61556158
for (Object[] entry: new Object[][] { { "versionTopic", localVt },
6156-
{ "consumedBytesSinceLastSync", consumedBytesMap },
61576159
{ "storageUtilizationManager", mock(StorageUtilizationManager.class) },
61586160
{ "partitionConsumptionStateMap", pcsMap } }) {
61596161
Field f = StoreIngestionTask.class.getDeclaredField((String) entry[0]);
@@ -6170,19 +6172,19 @@ public void testConsumedBytesSinceLastSyncTracking() throws Exception {
61706172

61716173
// Local VT: keyed by VT name
61726174
sit.produceToStoreBufferServiceOrKafka(records, new PubSubTopicPartitionImpl(localVt, partition), kafkaUrl, 0);
6173-
assertTrue(consumedBytesMap.containsKey(localVt.getName()), "Local VT should be tracked by VT name");
6174-
assertFalse(consumedBytesMap.containsKey(kafkaUrl));
6175+
verify(pcs).addConsumedBytesSinceLastGlobalRtDivSync(eq(localVt.getName()), anyLong());
6176+
verify(pcs, never()).addConsumedBytesSinceLastGlobalRtDivSync(eq(kafkaUrl), anyLong());
61756177

61766178
// RT topic: keyed by kafkaUrl
6177-
consumedBytesMap.clear();
6179+
clearInvocations(pcs);
61786180
sit.produceToStoreBufferServiceOrKafka(records, new PubSubTopicPartitionImpl(rtTopic, partition), kafkaUrl, 0);
6179-
assertTrue(consumedBytesMap.containsKey(kafkaUrl), "RT topic should be tracked by kafkaUrl");
6180-
assertFalse(consumedBytesMap.containsKey(rtTopic.getName()));
6181+
verify(pcs).addConsumedBytesSinceLastGlobalRtDivSync(eq(kafkaUrl), anyLong());
6182+
verify(pcs, never()).addConsumedBytesSinceLastGlobalRtDivSync(eq(rtTopic.getName()), anyLong());
61816183

61826184
// Remote VT: excluded entirely
6183-
consumedBytesMap.clear();
6185+
clearInvocations(pcs);
61846186
sit.produceToStoreBufferServiceOrKafka(records, new PubSubTopicPartitionImpl(remoteVt, partition), kafkaUrl, 0);
6185-
assertTrue(consumedBytesMap.isEmpty(), "Remote VT should not be tracked");
6187+
verify(pcs, never()).addConsumedBytesSinceLastGlobalRtDivSync(any(), anyLong());
61866188
}
61876189

61886190
/**

0 commit comments

Comments
 (0)