feat: Support timestamp protobuf serde #6927
Conversation
|
cc @rayokota |
| final ProtobufSchema resolved = AbstractKafkaProtobufSerializer.resolveDependencies( | ||
| srClient, | ||
| true, | ||
| true, |
There was a problem hiding this comment.
nit: the arg for useLatestVersion is not used because autoRegisterSchema is true, but it would probably be best to pass false anyway (since false is the default for useLatestVersion in general). So the args should read srClient, true, false, true.
There was a problem hiding this comment.
@rayokota - looking at AbstractKafkProtobufSerializer I noticed this method is "Visible for testing". If we're going to be depending on it here, can we remove that comment and make it clear that it's public API? Also would be nice to document it since I'm not entirely sure what it does from the method name. Does it just flatten it into one big protobuf schema?
There was a problem hiding this comment.
@agavra , sure I've filed https://confluentinc.atlassian.net/browse/DGS-1285 to track. The method recursively registers referenced schemas.
| final ProtobufSchema resolved = AbstractKafkaProtobufSerializer.resolveDependencies( | ||
| srClient, | ||
| true, | ||
| true, |
There was a problem hiding this comment.
@rayokota - looking at AbstractKafkProtobufSerializer I noticed this method is "Visible for testing". If we're going to be depending on it here, can we remove that comment and make it clear that it's public API? Also would be nice to document it since I'm not entirely sure what it does from the method name. Does it just flatten it into one big protobuf schema?
Description
One of the reasons the decimal and timestamp types were not enabled in protobuf was because those two types have dependencies that need to be registered first in order for schema registry to be able to read schemas involving these types. This PR adds an extra step before registering protobuf schemas that resolves dependencies.
Testing done
manually, unit and QTT tests
Reviewer checklist