Copilot commented on code in PR #863:
URL:
https://github.com/apache/skywalking-banyandb/pull/863#discussion_r2560387921
##########
api/proto/banyandb/trace/v1/write.proto:
##########
@@ -26,11 +26,16 @@ import "validate/validate.proto";
option go_package =
"github.com/apache/skywalking-banyandb/api/proto/banyandb/trace/v1";
option java_package = "org.apache.skywalking.banyandb.trace.v1";
+message TagSpec {
Review Comment:
The `TagSpec` message is missing documentation comments. Similar messages in
other proto files (e.g., `TagFamilySpec` in stream/v1/write.proto and
measure/v1/write.proto) include descriptive comments explaining the message
purpose and its fields. Consider adding comments like:
```
// TagSpec defines the specification of tags.
message TagSpec {
// names of tags
repeated string tag_names = 1;
}
```
```suggestion
// TagSpec defines the specification of tags.
message TagSpec {
// names of tags
```
##########
api/proto/banyandb/stream/v1/write.proto:
##########
@@ -27,14 +27,22 @@ import "validate/validate.proto";
option go_package =
"github.com/apache/skywalking-banyandb/api/proto/banyandb/stream/v1";
option java_package = "org.apache.skywalking.banyandb.stream.v1";
+// TagFamilySpec defines the specification of a tag family.
+message TagFamilySpec {
+ // name of the tag family
+ string name = 1;
+ // names of tags in the tag family
+ repeated string tag_names = 2;
+}
Review Comment:
The `TagFamilySpec` message is duplicated between stream/v1/write.proto and
measure/v1/write.proto with identical structure. Consider defining this message
once in a common location (e.g., banyandb/model/v1/write.proto) and importing
it in both stream and measure to avoid duplication and ensure consistency. This
improves maintainability as any future changes to TagFamilySpec would only need
to be made in one place.
##########
api/proto/banyandb/trace/v1/write.proto:
##########
@@ -26,11 +26,16 @@ import "validate/validate.proto";
option go_package =
"github.com/apache/skywalking-banyandb/api/proto/banyandb/trace/v1";
option java_package = "org.apache.skywalking.banyandb.trace.v1";
+message TagSpec {
+ repeated string tag_names = 1;
+}
+
message WriteRequest {
common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
repeated model.v1.TagValue tags = 2 [(validate.rules).repeated.min_items =
1];
bytes span = 3;
uint64 version = 4 [(validate.rules).uint64.gt = 0];
Review Comment:
The `tag_spec` field is missing a documentation comment. Other similar
fields in stream/v1/write.proto and measure/v1/write.proto have descriptive
comments. Consider adding a comment like:
```
// the tag specification.
TagSpec tag_spec = 5;
```
```suggestion
uint64 version = 4 [(validate.rules).uint64.gt = 0];
// the tag specification.
```
##########
api/proto/banyandb/trace/v1/write.proto:
##########
@@ -26,11 +26,16 @@ import "validate/validate.proto";
option go_package =
"github.com/apache/skywalking-banyandb/api/proto/banyandb/trace/v1";
option java_package = "org.apache.skywalking.banyandb.trace.v1";
+message TagSpec {
+ repeated string tag_names = 1;
+}
+
message WriteRequest {
common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
Review Comment:
The `tags` field should have a comment clarifying the ordering behavior when
`tag_spec` is provided, similar to how it's documented in stream/v1/write.proto
and measure/v1/write.proto. Consider adding a comment like:
```
// the order of tags' items match TagSpec if provided, otherwise match the
trace schema
repeated model.v1.TagValue tags = 2 [(validate.rules).repeated.min_items =
1];
```
```suggestion
common.v1.Metadata metadata = 1 [(validate.rules).message.required = true];
// the order of tags' items match TagSpec if provided, otherwise match the
trace schema
```
##########
api/proto/banyandb/measure/v1/write.proto:
##########
@@ -27,13 +27,29 @@ import "validate/validate.proto";
option go_package =
"github.com/apache/skywalking-banyandb/api/proto/banyandb/measure/v1";
option java_package = "org.apache.skywalking.banyandb.measure.v1";
+// TagFamilySpec defines the specification of a tag family.
+message TagFamilySpec {
+ // name of the tag family
+ string name = 1;
+ // names of tags in the tag family
+ repeated string tag_names = 2;
+}
Review Comment:
The `TagFamilySpec` message is duplicated between stream/v1/write.proto and
measure/v1/write.proto with identical structure. Consider defining this message
once in a common location (e.g., banyandb/model/v1/write.proto) and importing
it in both stream and measure to avoid duplication and ensure consistency. This
improves maintainability as any future changes to TagFamilySpec would only need
to be made in one place.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
