Skip to content

Commit 1a5a387

Browse files
KaiSernLimCopilot
andcommitted
[dvc] Rehydrate remote LCVP on restart and guard empty-buffer resolution
Address PR review feedback on the Global RT DIV remote-LCVP resume path: - OffsetRecord#getLatestConsumedRemoteVtPosition now resolves a null/empty wire-format buffer to EARLIEST instead of deserializing an empty payload (which threw for pre-v24 persisted PartitionState records). - PartitionTracker#setPartitionState rehydrates latestConsumedRemoteVtPosition from the durable OffsetRecord for the version topic on restart, so a follower's VT-DIV checkpoint no longer clobbers the persisted remote LCVP back to EARLIEST before an F->L resume. - Fix TestAvroSchema#testStringMapInPartitionState to set the new v24 upstreamLastConsumedVersionTopicPubSubPosition field. - Add regression tests for both fixes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
1 parent 7310545 commit 1a5a387

5 files changed

Lines changed: 52 additions & 2 deletions

File tree

clients/da-vinci-client/src/main/java/com/linkedin/davinci/validation/PartitionTracker.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,12 @@ public void setPartitionState(TopicType type, OffsetRecord offsetRecord, long ma
179179
long earliestAllowableTimestamp =
180180
computeEarliestAllowableTimestamp(maxAgeInMs, offsetRecord.calculateLatestMessageTimeInMs());
181181
setPartitionState(type, offsetRecord.getProducerPartitionStateMap(), earliestAllowableTimestamp);
182+
if (TopicType.isVersionTopic(type)) {
183+
// Rehydrate the durable remote LCVP so a follower's VT-DIV checkpoint writes back the persisted value instead of
184+
// EARLIEST across a restart. Unlike the local LCVP (re-advanced live on the consume path), the remote LCVP is
185+
// only advanced by a remote-consume leader, so without this it would collapse to EARLIEST before an F->L resume.
186+
updateLatestConsumedRemoteVtPosition(offsetRecord.getLatestConsumedRemoteVtPosition());
187+
}
182188
}
183189

184190
public void setPartitionState(

clients/da-vinci-client/src/test/java/com/linkedin/davinci/validation/TestPartitionTracker.java

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import com.linkedin.venice.pubsub.adapter.kafka.common.ApacheKafkaOffsetPosition;
4242
import com.linkedin.venice.pubsub.api.DefaultPubSubMessage;
4343
import com.linkedin.venice.pubsub.api.PubSubPosition;
44+
import com.linkedin.venice.pubsub.api.PubSubSymbolicPosition;
4445
import com.linkedin.venice.pubsub.api.PubSubTopic;
4546
import com.linkedin.venice.pubsub.api.PubSubTopicPartition;
4647
import com.linkedin.venice.pubsub.mock.InMemoryPubSubPosition;
@@ -772,6 +773,29 @@ public void testUpdateOffsetRecordPersistsRemoteLcvp() {
772773
assertEquals(destTracker.getLatestConsumedRemoteVtPosition(), remoteLcvp, "remote LCVP should be copied on clone");
773774
}
774775

776+
/**
777+
* On restart, {@link PartitionTracker#setPartitionState(TopicType, OffsetRecord, long)} must rehydrate the remote
778+
* LCVP from the durable OffsetRecord. Otherwise a follower's VT-DIV checkpoint would write the in-memory EARLIEST
779+
* back over the persisted value, collapsing the F->L remote-VT resume position to EARLIEST across any bounce.
780+
*/
781+
@Test(timeOut = 10 * Time.MS_PER_SECOND)
782+
public void testSetPartitionStateRehydratesRemoteLcvp() {
783+
PubSubPosition remoteLcvp = ApacheKafkaOffsetPosition.of(100L);
784+
OffsetRecord offsetRecord = TestUtils
785+
.getOffsetRecord(ApacheKafkaOffsetPosition.of(0L), Optional.empty(), DEFAULT_PUBSUB_CONTEXT_FOR_UNIT_TESTING);
786+
offsetRecord.setLatestConsumedRemoteVtPosition(remoteLcvp);
787+
788+
// Fresh tracker starts at EARLIEST, mirroring a restart before any remote-VT record is consumed.
789+
PartitionTracker restartedTracker = createDestTracker();
790+
assertEquals(restartedTracker.getLatestConsumedRemoteVtPosition(), PubSubSymbolicPosition.EARLIEST);
791+
792+
restartedTracker.setPartitionState(vt, offsetRecord, MAX_AGE_IN_MS);
793+
assertEquals(
794+
restartedTracker.getLatestConsumedRemoteVtPosition(),
795+
remoteLcvp,
796+
"Remote LCVP must be rehydrated from the OffsetRecord on restart so it is not clobbered back to EARLIEST");
797+
}
798+
775799
/**
776800
* Tests that cloneVtProducerStates correctly handles the maxAgeInMs threshold and data-relative anchor:
777801
* <ul>

internal/venice-common/src/main/java/com/linkedin/venice/offsets/OffsetRecord.java

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -394,8 +394,13 @@ public void setLatestConsumedRemoteVtPosition(PubSubPosition latestConsumedRemot
394394
}
395395

396396
public PubSubPosition getLatestConsumedRemoteVtPosition() {
397-
return pubSubPositionDeserializer
398-
.toPosition(this.partitionState.getUpstreamLastConsumedVersionTopicPubSubPosition());
397+
ByteBuffer wireFormat = this.partitionState.getUpstreamLastConsumedVersionTopicPubSubPosition();
398+
// Records persisted before PartitionState v24 lack this field, so Avro schema resolution supplies the field
399+
// default (empty bytes). Treat null/empty as EARLIEST rather than deserializing an empty payload, which throws.
400+
if (wireFormat == null || !wireFormat.hasRemaining()) {
401+
return PubSubSymbolicPosition.EARLIEST;
402+
}
403+
return pubSubPositionDeserializer.toPosition(wireFormat);
399404
}
400405

401406
public long getActiveKeyCount() {

internal/venice-common/src/test/java/com/linkedin/venice/offsets/TestOffsetRecord.java

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import static org.testng.Assert.assertTrue;
77

88
import com.linkedin.venice.kafka.protocol.GUID;
9+
import com.linkedin.venice.kafka.protocol.state.PartitionState;
910
import com.linkedin.venice.kafka.protocol.state.ProducerPartitionState;
1011
import com.linkedin.venice.pubsub.adapter.kafka.common.ApacheKafkaOffsetPosition;
1112
import com.linkedin.venice.pubsub.api.PubSubPosition;
@@ -64,6 +65,19 @@ public void testToBytes() {
6465
Assert.assertEquals(offsetRecord2, offsetRecord1);
6566
}
6667

68+
@Test
69+
public void testLatestConsumedRemoteVtPositionEmptyBufferDefaultsToEarliest() {
70+
// Simulates a pre-v24 persisted record: Avro schema resolution supplies the field default (empty bytes) for the
71+
// newly added field. The getter must resolve that to EARLIEST rather than deserializing an empty payload.
72+
PartitionState partitionState = new PartitionState();
73+
partitionState.upstreamLastConsumedVersionTopicPubSubPosition = ByteBuffer.allocate(0);
74+
OffsetRecord offsetRecord = new OffsetRecord(
75+
partitionState,
76+
AvroProtocolDefinition.PARTITION_STATE.getSerializer(),
77+
DEFAULT_PUBSUB_CONTEXT_FOR_UNIT_TESTING);
78+
assertEquals(offsetRecord.getLatestConsumedRemoteVtPosition(), PubSubSymbolicPosition.EARLIEST);
79+
}
80+
6781
@Test
6882
public void testLatestConsumedRemoteVtPositionRoundTrip() {
6983
OffsetRecord fresh = new OffsetRecord(

internal/venice-common/src/test/java/com/linkedin/venice/schema/TestAvroSchema.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ public void testStringMapInPartitionState() throws IOException {
7575
ps.lastConsumedVersionTopicPubSubPosition = ByteBuffer.wrap("".getBytes());
7676
ps.upstreamRealTimeTopicPubSubPositionMap = new VeniceConcurrentHashMap<>();
7777
ps.upstreamVersionTopicPubSubPosition = ByteBuffer.wrap("".getBytes());
78+
ps.upstreamLastConsumedVersionTopicPubSubPosition = ByteBuffer.wrap("".getBytes());
7879

7980
AvroSerializer serializer = new AvroSerializer(ps.getSchema());
8081
byte[] serializedBytes = serializer.serialize(ps);

0 commit comments

Comments
 (0)