-
Notifications
You must be signed in to change notification settings - Fork 86
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
[dvc][common][test] leader complete status to follower part 1: leader produce HB and SOS to local VT with new header #741
Conversation
…to local VT with new header
...linkedin/venice/restart/TestRestartServerAfterDeletingSstFilesWithActiveActiveIngestion.java
Outdated
Show resolved
Hide resolved
.../main/java/com/linkedin/venice/pubsub/adapter/kafka/consumer/ApacheKafkaConsumerAdapter.java
Outdated
Show resolved
Hide resolved
...-client/src/main/java/com/linkedin/davinci/consumer/ImmutableChangeCapturePubSubMessage.java
Outdated
Show resolved
Hide resolved
...c/main/java/com/linkedin/venice/pubsub/adapter/kafka/consumer/ApacheKafkaConsumerConfig.java
Outdated
Show resolved
Hide resolved
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.
Thanks @sushantmane for the review. Addressed review comments.
...linkedin/venice/restart/TestRestartServerAfterDeletingSstFilesWithActiveActiveIngestion.java
Outdated
Show resolved
Hide resolved
...-client/src/main/java/com/linkedin/davinci/consumer/ImmutableChangeCapturePubSubMessage.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubMessage.java
Outdated
Show resolved
Hide resolved
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.
I've left two comments. I think it would be good to have this PR reviewed by @FelixGV as well. Thanks!
...al/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubMessageDeserializer.java
Outdated
Show resolved
Hide resolved
...ient/src/main/java/com/linkedin/davinci/kafka/consumer/LeaderFollowerStoreIngestionTask.java
Outdated
Show resolved
Hide resolved
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.
Thanks, LGTM overall. Left a few minor comments.
...ient/src/main/java/com/linkedin/davinci/kafka/consumer/LeaderFollowerStoreIngestionTask.java
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/ImmutablePubSubMessage.java
Show resolved
Hide resolved
...ient/src/main/java/com/linkedin/davinci/kafka/consumer/LeaderFollowerStoreIngestionTask.java
Outdated
Show resolved
Hide resolved
...ts/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java
Outdated
Show resolved
Hide resolved
...ts/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java
Outdated
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java
Outdated
Show resolved
Hide resolved
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.
Thanks @gaojieliu and @FelixGV for the review. Addressed review comments. Please take another look at it.
internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java
Outdated
Show resolved
Hide resolved
...ts/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java
Outdated
Show resolved
Hide resolved
...ts/da-vinci-client/src/main/java/com/linkedin/davinci/kafka/consumer/StoreIngestionTask.java
Outdated
Show resolved
Hide resolved
...ient/src/main/java/com/linkedin/davinci/kafka/consumer/LeaderFollowerStoreIngestionTask.java
Outdated
Show resolved
Hide resolved
...ient/src/main/java/com/linkedin/davinci/kafka/consumer/LeaderFollowerStoreIngestionTask.java
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/pubsub/ImmutablePubSubMessage.java
Show resolved
Hide resolved
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.
Looks good overall. Just a minor nitpick...
internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java
Outdated
Show resolved
Hide resolved
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.
Please update the commit message to describe the updated behavior that the leader complete header will only be sent in HB SOS message.
Also, I didn't see any test cases for the new logic, so is it expected? or you want to keep all the test cases in the second PR?
internal/venice-common/src/main/java/com/linkedin/venice/writer/VeniceWriter.java
Show resolved
Hide resolved
internal/venice-common/src/main/java/com/linkedin/venice/writer/LeaderCompleteState.java
Outdated
Show resolved
Hide resolved
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.
LGTM, but please check Gaojie's latest comments as well in case there is something to address there.
Summary
Issue: For hybrid stores, followers are marked completed once EOP in VT is received and before leaders are marked complete and when leader handover happens to one of the completed follower instances, even though the ingestion is not completed, it will be marked completed and ready to serve.
Fix:
Part 1:
The proposal is to have a new
isLeaderCompleted
Kafka header which can be added to heartbeatSOS
messages. The value of that header would be:0 or 1. As soon as the leader partition determines that it is complete (according to whatever rule it determines completion by, usually offset lag, but could be time lag also) the leader will then immediately send a heartbeat message containing the header:isLeaderCompleted
= 1. All future heartbeats will also contain that same header.Implementation details of part 1:
PubSubMessageHeader
inPubSubMessageHeaders
to hold this info in HB messages sent from leader to VT. This information won't be produced to RT.PubSubMessageHeaders
will be added as a part ofPubSubMessage
during deserialization of the consumed records to be later used by the followers in part 2.PartitionConsumptionState#isCompletionReported()
to determine whether leader is marked completed or not and piggybacking onreportCompleted()
to initiate heartbeat message immediately once leader is marked completed.Part 2 (TBD):
This includes followers reading the header added in part 1 and using that as an additional factor to decide whether it should be completed or not.
How was this PR tested?
Tested whether the new header is being received by consuming the version topic in an integration test
Does this PR introduce any user-facing changes?