Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
trollyxia
changed the title
| } | ||
|
|
||
| /** Sets the avro schema for this schema bundle. */ | ||
| public CreateSchemaBundleRequest setAvroSchema(@Nonnull List<String> avroSchema) { |
There was a problem hiding this comment.
proto schema doesn't have a list override, why is it needed for avro schema?
There was a problem hiding this comment.
Because their proto definitions differ: http://google3/google/bigtable/admin/v2/table.proto;l=1121-1157;rcl=974104276!
i.e., ProtoSchema defines a single bytes proto_descriptors field, whereas AvroSchema defines a repeated string json_schemas field to accept multiple schema files in a bundle.
| Preconditions.checkArgument( | ||
| proto.hasProtoSchema(), "Schemabundle must have a proto_schema field"); | ||
| proto.hasProtoSchema() || proto.hasAvroSchema(), | ||
| "Schemabundle must have a proto_schema or avro_schema field"); |
There was a problem hiding this comment.
is proto.hasProtoSchema && proto.hasAvroSchema() also invalid?
There was a problem hiding this comment.
In API definition,
proto_schema and avro_schema are defined inside a oneof:
oneof type {
ProtoSchema proto_schema;
AvroSchema avro_schema;
}
If the GAPIC correctly translates that into the Java classes, I think there won't be a schema bundle with proto.hasProtoSchema && proto.hasAvroSchema()
There was a problem hiding this comment.
Also in this case, in CreateSchemaBundle, should we validate that avro schema is not set when setting proto schema, vice versa?
| Preconditions.checkArgument( | ||
| proto.hasProtoSchema(), "Schemabundle must have a proto_schema field"); | ||
| proto.hasProtoSchema() || proto.hasAvroSchema(), | ||
| "Schemabundle must have a proto_schema or avro_schema field"); |
| Preconditions.checkArgument( | ||
| proto.hasProtoSchema(), "Schemabundle must have a proto_schema field"); | ||
| proto.hasProtoSchema() || proto.hasAvroSchema(), | ||
| "Schemabundle must have a proto_schema or avro_schema field"); |
There was a problem hiding this comment.
Also in this case, in CreateSchemaBundle, should we validate that avro schema is not set when setting proto schema, vice versa?
| requestBuilder.setSchemaBundle( | ||
| com.google.bigtable.admin.v2.SchemaBundle.newBuilder() | ||
| .setProtoSchema(ProtoSchema.newBuilder().setProtoDescriptors(protoSchema))); | ||
| requestBuilder |
There was a problem hiding this comment.
should we validate that avro schema is not set?
|
|
||
| /** 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.
2 participants
No description provided.