-
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
[changelog][server] Fix incoming write schema id #735
Conversation
This is to fix a WC/schema evolution bug. Tests to follow. But the changecapture should contain the schema of the merge result which should be the superset schema not the write compute one.
...client/src/main/java/com/linkedin/davinci/kafka/consumer/ActiveActiveStoreIngestionTask.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! This looks like a great improvement! I left some minor comments that are mostly cosmetic, though I hope we could address them.
...da-vinci-client/src/main/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerImpl.java
Show resolved
Hide resolved
...-client/src/main/java/com/linkedin/davinci/storage/chunking/AbstractAvroChunkingAdapter.java
Outdated
Show resolved
Hide resolved
...s/da-vinci-client/src/main/java/com/linkedin/davinci/store/record/ByteBufferValueRecord.java
Outdated
Show resolved
Hide resolved
...client/src/main/java/com/linkedin/davinci/kafka/consumer/ActiveActiveStoreIngestionTask.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, LFG!!!!! 🚀🚀🚀🚀🚀🚀🚀
So... I think we need to do some code coverage for this 🤷 ... |
Fixed |
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!
This fixes a regression introduced in PR linkedin#735.
This fixes a regression introduced in PR #735.
This fixes a regression introduced in PR #735.
This fixes a regression introduced in PR linkedin#735.
This is to fix a WC/schema evolution bug. Tests to follow. But the changecapture should contain the schema of the merge result which should be the superset schema not the write compute one.
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?