-
Notifications
You must be signed in to change notification settings - Fork 1.1k
DGS-20677: RM Metadata Encoder Integration [do not merge] #3737
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
base: 7.7.x
Are you sure you want to change the base?
Conversation
🎉 All Contributor License Agreements have been signed. Ready to merge. |
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.
Pull Request Overview
This PR adds an interface for the MetadataEncoderService to allow key functionality to be overridden, ensuring improved flexibility on CCloud. Key changes include:
- Creation of MetadataEncoderServiceInterface with essential metadata encoding methods.
- Updated MetadataEncoderService implementing the new interface with changes to support a rotationNeeded flag.
- Modification of KafkaSchemaRegistry to utilize the new interface and instantiate MetadataEncoderService via a factory method.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
core/src/main/java/io/confluent/kafka/schemaregistry/storage/encoder/MetadataEncoderServiceInterface.java | New interface defining metadata encoding and transformation methods. |
core/src/main/java/io/confluent/kafka/schemaregistry/storage/encoder/MetadataEncoderService.java | Updated service implementation to include new interface and rotationNeeded logic. |
core/src/main/java/io/confluent/kafka/schemaregistry/storage/KafkaSchemaRegistry.java | Refactored to use the new interface and updated instantiation of the encoder service. |
@@ -350,7 +355,7 @@ public Serializer<SchemaRegistryKey, SchemaRegistryValue> getSerializer() { | |||
} | |||
|
|||
public MetadataEncoderService getMetadataEncoder() { | |||
return metadataEncoder; | |||
return (MetadataEncoderService) metadataEncoder; |
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.
[nitpick] Consider returning the service as a MetadataEncoderServiceInterface rather than casting to the concrete type to preserve abstraction.
Copilot uses AI. Check for mistakes.
import java.util.concurrent.atomic.AtomicBoolean; | ||
import java.util.function.BiFunction; | ||
|
||
public interface MetadataEncoderServiceInterface { |
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.
[nitpick] Consider adding JavaDoc comments to the interface methods to better document expected behaviors and usage for implementers.
Copilot uses AI. Check for mistakes.
What's the purpose of this PR? It seems you're also changing the behavior by enabling rotation |
What
In order to allow for overriding of some key functionality of the MetadataEncoderService on CCloud, an interface has been created for the class, with the default implementation remaining as is.
Checklist
Please answer the questions with Y, N or N/A if not applicable.
References
JIRA:
Test & Review
Open questions / Follow-ups