-
Notifications
You must be signed in to change notification settings - Fork 114
[da-vinci] Stop populating replication_checkpoint_vector and remove offset-vector utility methods #2479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[da-vinci] Stop populating replication_checkpoint_vector and remove offset-vector utility methods #2479
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -495,8 +495,6 @@ private PubSubMessageProcessedResult processActiveActiveMessage( | |||||||||||||||||||||||||||||||||||
| beforeProcessingBatchRecordsTimestampMs); | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| final long writeTimestamp = getWriteTimestampFromKME(kafkaValue); | ||||||||||||||||||||||||||||||||||||
| final long offsetSumPreOperation = | ||||||||||||||||||||||||||||||||||||
| rmdWithValueSchemaID != null ? RmdUtils.extractOffsetVectorSumFromRmd(rmdWithValueSchemaID.getRmdRecord()) : 0; | ||||||||||||||||||||||||||||||||||||
| List<Long> recordTimestampsPreOperation = rmdWithValueSchemaID != null | ||||||||||||||||||||||||||||||||||||
| ? RmdUtils.extractTimestampFromRmd(rmdWithValueSchemaID.getRmdRecord()) | ||||||||||||||||||||||||||||||||||||
| : Collections.singletonList(0L); | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
498
to
500
|
||||||||||||||||||||||||||||||||||||
| List<Long> recordTimestampsPreOperation = rmdWithValueSchemaID != null | |
| ? RmdUtils.extractTimestampFromRmd(rmdWithValueSchemaID.getRmdRecord()) | |
| : Collections.singletonList(0L); |
Copilot
AI
Feb 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validatePostOperationResultsAndRecord has become a no-op: if the update is not ignored, the method still performs no validation or metrics/logging. If the offset/timestamp checks are intentionally removed, consider deleting this method and its call site to avoid dead code; otherwise, keep the remaining validation here.
| } | |
| } | |
| // Basic sanity checks and diagnostics for applied updates. | |
| if (timestampsPreOperation == null) { | |
| if (LOGGER.isWarnEnabled()) { | |
| LOGGER.warn("timestampsPreOperation is null while processing a non-ignored merge result: {}", mergeConflictResult); | |
| } | |
| return; | |
| } | |
| if (LOGGER.isDebugEnabled()) { | |
| LOGGER.debug( | |
| "Validated post-operation merge result. Update applied: {}, pre-operation timestamps count: {}", | |
| !mergeConflictResult.isUpdateIgnored(), | |
| timestampsPreOperation.size()); | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,7 @@ | |||||||||||
|
|
||||||||||||
| import com.linkedin.davinci.schema.merge.ValueAndRmd; | ||||||||||||
| import com.linkedin.venice.pubsub.api.PubSubPosition; | ||||||||||||
| import java.util.List; | ||||||||||||
| import java.util.ArrayList; | ||||||||||||
| import org.apache.avro.generic.GenericRecord; | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
|
|
@@ -28,7 +28,7 @@ protected ValueAndRmd<T> putWithRecordLevelTimestamp( | |||||||||||
| // New value wins | ||||||||||||
| oldValueAndRmd.setValue(newValue); | ||||||||||||
| oldRmd.put(TIMESTAMP_FIELD_POS, putOperationTimestamp); | ||||||||||||
| updateReplicationCheckpointVector(oldRmd, newValueSourcePosition, newValueSourceBrokerID); | ||||||||||||
| updateReplicationCheckpointVector(oldRmd); | ||||||||||||
|
|
||||||||||||
| } else if (oldTimestamp == putOperationTimestamp) { | ||||||||||||
| // When timestamps tie, compare decide which one should win. | ||||||||||||
|
|
@@ -38,7 +38,7 @@ protected ValueAndRmd<T> putWithRecordLevelTimestamp( | |||||||||||
| oldValueAndRmd.setUpdateIgnored(true); | ||||||||||||
| } else { | ||||||||||||
| oldValueAndRmd.setValue(newValue); | ||||||||||||
| updateReplicationCheckpointVector(oldRmd, newValueSourcePosition, newValueSourceBrokerID); | ||||||||||||
| updateReplicationCheckpointVector(oldRmd); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| } else { | ||||||||||||
|
|
@@ -60,24 +60,16 @@ protected ValueAndRmd<T> deleteWithValueLevelTimestamp( | |||||||||||
| // Still need to track the delete timestamp in order to reject future PUT record with lower replication timestamp | ||||||||||||
| final GenericRecord oldRmd = oldValueAndRmd.getRmd(); | ||||||||||||
| oldRmd.put(TIMESTAMP_FIELD_POS, deleteOperationTimestamp); | ||||||||||||
| updateReplicationCheckpointVector(oldRmd, newValueSourcePosition, newValueSourceBrokerID); | ||||||||||||
| updateReplicationCheckpointVector(oldRmd); | ||||||||||||
|
|
||||||||||||
| } else { | ||||||||||||
| oldValueAndRmd.setUpdateIgnored(true); | ||||||||||||
| } | ||||||||||||
| return oldValueAndRmd; | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| protected void updateReplicationCheckpointVector( | ||||||||||||
| GenericRecord oldRmd, | ||||||||||||
| PubSubPosition newValueSourcePosition, | ||||||||||||
| int newValueSourceBrokerID) { | ||||||||||||
| oldRmd.put( | ||||||||||||
| REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, | ||||||||||||
| MergeUtils.mergeOffsetVectors( | ||||||||||||
| (List<Long>) oldRmd.get(REPLICATION_CHECKPOINT_VECTOR_FIELD_POS), | ||||||||||||
| newValueSourcePosition.getNumericOffset(), | ||||||||||||
| newValueSourceBrokerID)); | ||||||||||||
| protected void updateReplicationCheckpointVector(GenericRecord oldRmd) { | ||||||||||||
| oldRmd.put(REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, new ArrayList<>()); | ||||||||||||
|
||||||||||||
| oldRmd.put(REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, new ArrayList<>()); | |
| Object existingVector = oldRmd.get(REPLICATION_CHECKPOINT_VECTOR_FIELD_POS); | |
| if (existingVector == null) { | |
| oldRmd.put(REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, java.util.Collections.emptyList()); | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -575,10 +575,7 @@ private MergeConflictResult putWithoutRmd( | |
| */ | ||
| GenericRecord newRmd = newRmdCreator.apply(newValueSchemaID); | ||
| newRmd.put(TIMESTAMP_FIELD_POS, putOperationTimestamp); | ||
| // A record which didn't come from an RT topic or has null metadata should have no offset vector. | ||
| newRmd.put( | ||
| REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, | ||
| MergeUtils.mergeOffsetVectors(null, newValueSourcePosition.getNumericOffset(), newValueSourceBrokerID)); | ||
| newRmd.put(REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, new ArrayList<>()); | ||
|
|
||
|
Comment on lines
576
to
579
|
||
| if (useFieldLevelTimestamp) { | ||
| Schema valueSchema = getValueSchema(newValueSchemaID); | ||
|
|
@@ -603,9 +600,7 @@ private MergeConflictResult deleteWithoutRmd( | |
| final int valueSchemaID = storeSchemaCache.getSupersetOrLatestValueSchema().getId(); | ||
| GenericRecord newRmd = newRmdCreator.apply(valueSchemaID); | ||
| newRmd.put(TIMESTAMP_FIELD_POS, deleteOperationTimestamp); | ||
| newRmd.put( | ||
| REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, | ||
| MergeUtils.mergeOffsetVectors(null, newValueSourcePosition.getNumericOffset(), deleteOperationSourceBrokerID)); | ||
| newRmd.put(REPLICATION_CHECKPOINT_VECTOR_FIELD_POS, new ArrayList<>()); | ||
| if (useFieldLevelTimestamp) { | ||
| Schema valueSchema = getValueSchema(valueSchemaID); | ||
| newRmd = createOldValueAndRmd(valueSchema, valueSchemaID, valueSchemaID, Lazy.of(() -> null), newRmd).getRmd(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,7 +116,7 @@ private ValueAndRmd<GenericRecord> handlePutWithPerFieldLevelTimestamp( | |
| GenericRecord newValue) { | ||
| final GenericRecord oldReplicationMetadata = oldValueAndRmd.getRmd(); | ||
| final GenericRecord oldValue = oldValueAndRmd.getValue(); | ||
| updateReplicationCheckpointVector(oldReplicationMetadata, sourcePositionOfNewValue, newValueSourceBrokerID); | ||
| updateReplicationCheckpointVector(oldReplicationMetadata); | ||
|
|
||
|
Comment on lines
116
to
120
|
||
| List<Schema.Field> fieldsInNewRecord = newValue.getSchema().getFields(); | ||
| boolean noFieldUpdated = true; | ||
|
|
@@ -166,7 +166,7 @@ public ValueAndRmd<GenericRecord> delete( | |
| oldValueAndRmd); | ||
|
|
||
| case PER_FIELD_TIMESTAMP: | ||
| updateReplicationCheckpointVector(oldReplicationMetadata, newValueSourcePosition, newValueSourceBrokerID); | ||
| updateReplicationCheckpointVector(oldReplicationMetadata); | ||
| UpdateResultStatus recordDeleteResultStatus = mergeRecordHelper.deleteRecord( | ||
| oldValueAndRmd.getValue(), | ||
| (GenericRecord) tsObject, | ||
|
|
@@ -195,7 +195,7 @@ public ValueAndRmd<GenericRecord> update( | |
| int updateOperationColoID, | ||
| PubSubPosition newValueSourcePosition, | ||
| int newValueSourceBrokerID) { | ||
| updateReplicationCheckpointVector(oldValueAndRmd.getRmd(), newValueSourcePosition, newValueSourceBrokerID); | ||
| updateReplicationCheckpointVector(oldValueAndRmd.getRmd()); | ||
| return writeComputeProcessor.updateRecordWithRmd( | ||
| currValueSchema, | ||
| oldValueAndRmd, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The regression-prevention logic in this method is now inconsistent:
localOffsetis still computed (and the surrounding comments mention preventing high-watermark regressions), but thehasOffsetAdvancedguard was removed and the map is overwritten unconditionally. Either remove the stalelocalOffset/comment block, or restore a monotonicity check (e.g., viaDiffValidationUtils.hasOffsetAdvanced) before updatingcurrentVersionHighWatermarks.