-
Notifications
You must be signed in to change notification settings - Fork 85
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
[controller][test] Message header triggered new KME schema registrati… #628
Conversation
...al/venice-common/src/main/java/com/linkedin/venice/pubsub/api/PubSubMessageDeserializer.java
Show resolved
Hide resolved
...al/venice-common/src/main/java/com/linkedin/venice/helix/HelixReadWriteSchemaRepository.java
Outdated
Show resolved
Hide resolved
.../venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerService.java
Show resolved
Hide resolved
...Test/java/com/linkedin/venice/controller/TestControllerKMERegistrationFromMessageHeader.java
Outdated
Show resolved
Hide resolved
...Test/java/com/linkedin/venice/controller/TestControllerKMERegistrationFromMessageHeader.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 for making this change. I took a look at all code changes except for the new test class (will review the test after meetings).
Great work overall! Left two comments. Also, regarding the following commit message:
Otherwise, as KME is not embedded in every Kafka message, if servers restart and resume the consumption from a non-SOS message with new KME (for example, lor1 servers could consume TS sent by lva1 controller), servers can not find the right schema to deserialize it and keep failing.
I think this PR could not help with servers that encounter unknown KME after restart because this PR does not include any server change. It actually helps controllers which may encounter unknown KME after restarting and resuming admin message consumption. You may need to update the commit message to avoid confusion.
...ain/java/com/linkedin/venice/system/store/ControllerClientBackedSystemSchemaInitializer.java
Outdated
Show resolved
Hide resolved
...ain/java/com/linkedin/venice/system/store/ControllerClientBackedSystemSchemaInitializer.java
Outdated
Show resolved
Hide resolved
Thanks for review, good catch, will fix the description. |
…on in child controller As part of the solution to remove deployment order for KafkaMessageEnvelope (KME) value schema, for controllers, when they find an unknown KME schema from the message header, they need to (talk to the system cluster leader controller to) register the new schema into local colo system store. Otherwise, as KME is not embedded in every Kafka message, if servers restart and resume the consumption from a non-SOS message with new KME (for example, lor1 servers could consume TS sent by lva1 controller), servers can not find the right schema to deserialize it and keep failing. This rb enables the child controllers to register unknown KME schemas when discovering them from the message's header. It maily leverages the existing functionailities from the ControllerClientBackedSystemSchemaInitializer class for the implementation. A new config is introduced to enable this feature and a new integration test is added to verify the correctness of the new feature.
…on in child controller
As part of the solution to remove deployment order for KafkaMessageEnvelope (KME) value schema, for controllers, when they find an unknown KME schema from the message header, they need to (talk to the system cluster leader controller to) register the new schema into local region system store. It helps controllers to deal with unknown KME during admin message consumption.
This change enables the child controllers to register unknown KME schemas when discovering them from the message's header. It mainly leverages the existing functionalities from the ControllerClientBackedSystemSchemaInitializer class for the implementation. A new config is introduced to enable this feature and a new integration test is added to verify the correctness of the new feature.
How was this PR tested?
Added a new integration test.
Passed CI tests.
Does this PR introduce any user-facing changes?