Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for Avro schemas in SchemaBundle and its corresponding create and update request builders, along with comprehensive unit and integration tests. The review feedback suggests refining the exception message in SchemaBundle.getAvroSchema() to clearly state that the bundle does not contain an Avro schema, rather than using a generic and potentially misleading message about an invalid type.
|
|
||
| /** Sets the avro schema for this schema bundle. */ | ||
| public UpdateSchemaBundleRequest setAvroSchema(@Nonnull List<String> avroSchema) { | ||
| Preconditions.checkNotNull(avroSchema, "avroSchema must be set"); |
There was a problem hiding this comment.
should we validate proto schema is not set?
| return setAvroSchema(Collections.singletonList(avroSchema)); | ||
| } | ||
|
|
||
| /** Sets the avro schema for this schema bundle. */ |
There was a problem hiding this comment.
sets a list of avro schema for this schema bundle.
| @@ -93,6 +93,9 @@ public UpdateSchemaBundleRequest setProtoSchemaFile(@Nonnull String protoSchemaF | |||
There was a problem hiding this comment.
Maybe the comment should be:
Update the proto schema set on this schema bundle or something.
Also, do we allow a schema bundle to switch between proto schema <-> avro schema? If so, the validation shouldn't be a precondition check (throws error), instead it would be a warning, something along the lines of "This schema bundle already has aavro schema. setting proto schema will unset the avro schema .. blah blah blah"
There was a problem hiding this comment.
Updated comment.
Regarding switching between proto_schema and avro_schema: The backend forbids changing the schema bundle type after creation, but that enforcement is only on the server-side.
The client can only help catch stupid mistakes like chaining .setProtoSchema(...).setAvroSchema(...), in which case the result is still a valid request with an Avro schema bundle, since the first call would be overwritten by the second call.
So it makes sense to degrade the error to a warning log - it warns the user that the previous schema type is being overwritten while staying aligned with standard protobuf oneof behavior. Updated both CreateSchemaBundleRequest and UpdateSchemaBundleRequest accordingly!
…eSchemaBundleRequest instead of throwing exceptions
|
Thank you, but please make sure to only merge when server side is ready. |
No description provided.