-
Notifications
You must be signed in to change notification settings - Fork 91
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
[changelog] Make changes for venice view consumption #1497
Conversation
[tests incoming] These changes make it so that the venice after image consumer is able to now consume view topics. View topics internally very much resemble version topics, which the after image consumer understands today. So the logic is largely the same. The only difference we add here is that we now have to consult store repositories which are view aware, and we have to chnage how we maintain local highwatermark information. highwatermarks are meant to be compared on RT partitions, and new view types map 1:N and N:N RT partitions to view partitions. This means we need to add an additional dimensionality to highwatermark filtering where instead of keeping a single map of topic partitions to offsets, but a map of topic partitions to maps of RT partitions to highwatermarks. There's still some pending work here, we need to figure out chunk assembly (as the current implementation of chunk assembly doesn't account for interleaving writes) and we need to put a bow on how we articulate to the consumers what the upstream RT partition is (right now we're just hardcoding the 1:1 pairing).
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.
Mostly questions since I'm still trying to understand the expected behavior of message filtering 🤓
...in/java/com/linkedin/davinci/consumer/InternalLocalBootstrappingVeniceChangelogConsumer.java
Outdated
Show resolved
Hide resolved
...client/src/main/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerClientFactory.java
Outdated
Show resolved
Hide resolved
...da-vinci-client/src/main/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerImpl.java
Outdated
Show resolved
Hide resolved
...da-vinci-client/src/main/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerImpl.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.
Just a question about usages of changelogClientConfig.getStoreName()
in InternalLocalBootstrappingVeniceChangelogConsumer
when view is specified.
...test-common/src/integrationTest/java/com/linkedin/venice/consumer/TestChangelogConsumer.java
Show resolved
Hide resolved
...da-vinci-client/src/main/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerImpl.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 Zac, but looks like unit test coverage police is on duty.
[tests incoming]
These changes make it so that the venice after image consumer is able to now consume view topics. View topics internally very much resemble version topics, which the after image consumer understands today. So the logic is largely the same. The only difference we add here is that we now have to consult store repositories which are view aware, and we have to chnage how we maintain local highwatermark information. Highwatermarks are meant to be compared on RT partitions, and new view types map 1:N and N:N RT partitions to view partitions. This means we need to add an additional dimensionality to highwatermark filtering where instead of keeping a single map of topic partitions to offsets, but a map of topic partitions to maps of RT partitions to highwatermarks.
There's still some pending work here, we need to figure out chunk assembly (as the current implementation of chunk assembly doesn't account for interleaving writes) and we need to put a bow on how we articulate to the consumers what the upstream RT partition is (right now we're just hardcoding the 1:1 pairing).
Summary, imperative, start upper case, don't end with a period
Resolves #XXX
How was this PR tested?
Does this PR introduce any user-facing changes?