Copilot commented on code in PR #913:
URL:
https://github.com/apache/skywalking-banyandb/pull/913#discussion_r2678284526
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -412,6 +579,23 @@ service GroupRegistryService {
// Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch
Get instead
rpc Exist(GroupRegistryServiceExistRequest) returns
(GroupRegistryServiceExistResponse);
+
+ // Inspect retrieves detailed information about a group including its
schemas, data distribution, and pending operations.
+ rpc Inspect(GroupRegistryServiceInspectRequest) returns
(GroupRegistryServiceInspectResponse) {
+ option (google.api.http) = {get: "/v1/group/content/{group}"};
+ }
+
+ // Delete removes a group and optionally all its associated data.
+ // Supports dry-run mode to preview what would be deleted.
+ // May return immediately with a task_id for background deletion of large
groups.
+ rpc Delete(GroupRegistryServiceDeleteRequest) returns
(GroupRegistryServiceDeleteResponse) {
+ option (google.api.http) = {delete: "/v1/group/{group}"};
Review Comment:
The HTTP path for the Delete endpoint has changed from
"/v1/group/schema/{group}" to "/v1/group/{group}". This is a breaking change
for existing API clients that are using the old path. If this is intentional,
it should be clearly documented in CHANGES.md as a breaking change.
```suggestion
option (google.api.http) = {
delete: "/v1/group/{group}"
additional_bindings {
delete: "/v1/group/schema/{group}"
}
};
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -353,12 +354,66 @@ message GroupRegistryServiceUpdateRequest {
message GroupRegistryServiceUpdateResponse {}
+// GroupRegistryServiceDeleteRequest is the request for deleting a group.
message GroupRegistryServiceDeleteRequest {
+ // group is the name of the group to delete.
string group = 1;
+ // dry_run indicates whether to perform a dry run without actually deleting
data.
+ // When true, returns what would be deleted without making changes.
+ bool dry_run = 2;
+ // force indicates whether to force delete the group even if it contains
data.
+ // When false, deletion will fail if the group is not empty.
+ bool force = 3;
}
+// GroupRegistryServiceDeleteResponse is the response for deleting a group.
message GroupRegistryServiceDeleteResponse {
+ // deleted indicates whether the group was deleted.
bool deleted = 1;
+ // task_id is the ID of the background deletion task.
+ string task_id = 2;
+}
+
+// GroupDeletionTask represents the status of a group deletion operation.
Review Comment:
The dry_run and force fields added to the DeleteRequest are not being used
in the existing Delete implementation. The current implementation in
banyand/liaison/grpc/registry.go only calls DeleteGroup with the group name,
ignoring these new parameters. Additionally, the task_id field in the response
is never populated. This means the API definition promises functionality that
isn't implemented.
```suggestion
// dry_run is currently reserved for future use.
// The server implementation currently ignores this field.
bool dry_run = 2;
// force is currently reserved for future use.
// The server implementation currently ignores this field.
bool force = 3;
}
// GroupRegistryServiceDeleteResponse is the response for deleting a group.
message GroupRegistryServiceDeleteResponse {
// deleted indicates whether the group was deleted, if supported by the
server.
bool deleted = 1;
// task_id is reserved for future use to track background deletion tasks.
// The server implementation currently does not populate this field.
string task_id = 2;
}
// GroupDeletionTask describes a potential background group deletion task.
// This message is currently reserved for future use and may not be
populated or
// returned by the server implementation yet.
```
##########
docs/api-reference.md:
##########
@@ -2704,12 +2793,13 @@ Type determine the index structure under the hood
<a name="banyandb-database-v1-GroupRegistryServiceDeleteResponse"></a>
### GroupRegistryServiceDeleteResponse
-
+GroupRegistryServiceDeleteResponse is the response for deleting a group.
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
-| deleted | [bool](#bool) | | |
+| deleted | [bool](#bool) | | deleted indicates whether the group was
deleted. |
+| task_id | [string](#string) | | task_id is the ID of the background
deletion task. |
Review Comment:
The documentation states that the Delete method "May return immediately with
a task_id for background deletion of large groups." However, the response
always includes a task_id field. The documentation should clarify when task_id
is populated (e.g., "Returns a task_id when deletion is performed in the
background") versus when deletion is synchronous (task_id empty). This
ambiguity could confuse API consumers about when to poll the Query endpoint.
```suggestion
| task_id | [string](#string) | | task_id is the ID of the background
deletion task when deletion is performed asynchronously; it is empty when
deletion completes synchronously, in which case no polling is required. |
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -412,6 +579,23 @@ service GroupRegistryService {
// Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch
Get instead
rpc Exist(GroupRegistryServiceExistRequest) returns
(GroupRegistryServiceExistResponse);
+
+ // Inspect retrieves detailed information about a group including its
schemas, data distribution, and pending operations.
+ rpc Inspect(GroupRegistryServiceInspectRequest) returns
(GroupRegistryServiceInspectResponse) {
+ option (google.api.http) = {get: "/v1/group/content/{group}"};
+ }
+
+ // Delete removes a group and optionally all its associated data.
+ // Supports dry-run mode to preview what would be deleted.
+ // May return immediately with a task_id for background deletion of large
groups.
+ rpc Delete(GroupRegistryServiceDeleteRequest) returns
(GroupRegistryServiceDeleteResponse) {
+ option (google.api.http) = {delete: "/v1/group/{group}"};
+ }
+
+ // Query retrieves the status of a group deletion task.
+ rpc Query(GroupRegistryServiceQueryRequest) returns
(GroupRegistryServiceQueryResponse) {
+ option (google.api.http) = {get: "/v1/group/task/{group}"};
+ }
}
Review Comment:
The order of RPC methods in the service has changed. The Delete method was
previously positioned after Update (before Get), but is now positioned after
Inspect (near the end). This reordering could affect generated client code and
documentation. If this change is intentional, it should be documented;
otherwise, consider maintaining the original method order to minimize
disruption.
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -353,12 +354,66 @@ message GroupRegistryServiceUpdateRequest {
message GroupRegistryServiceUpdateResponse {}
+// GroupRegistryServiceDeleteRequest is the request for deleting a group.
message GroupRegistryServiceDeleteRequest {
+ // group is the name of the group to delete.
string group = 1;
+ // dry_run indicates whether to perform a dry run without actually deleting
data.
+ // When true, returns what would be deleted without making changes.
+ bool dry_run = 2;
+ // force indicates whether to force delete the group even if it contains
data.
+ // When false, deletion will fail if the group is not empty.
+ bool force = 3;
}
+// GroupRegistryServiceDeleteResponse is the response for deleting a group.
message GroupRegistryServiceDeleteResponse {
+ // deleted indicates whether the group was deleted.
bool deleted = 1;
+ // task_id is the ID of the background deletion task.
+ string task_id = 2;
+}
+
+// GroupDeletionTask represents the status of a group deletion operation.
+message GroupDeletionTask {
+ // Phase represents the current phase of the deletion task.
+ enum Phase {
+ PHASE_UNSPECIFIED = 0;
+ // PHASE_PENDING indicates the task is waiting to start.
+ PHASE_PENDING = 1;
+ // PHASE_IN_PROGRESS indicates the task is currently executing.
+ PHASE_IN_PROGRESS = 2;
+ // PHASE_COMPLETED indicates the task has completed successfully.
+ PHASE_COMPLETED = 3;
+ // PHASE_FAILED indicates the task has failed.
+ PHASE_FAILED = 4;
+ }
+ // current_phase is the current phase of the deletion task.
+ Phase current_phase = 1;
+ // total_counts maps resource types to their total count.
+ map<string, int32> total_counts = 2;
+ // deleted_counts maps resource types to the count of deleted resources.
+ map<string, int32> deleted_counts = 3;
+ // total_data_size_bytes is the total size of data to be deleted in bytes.
+ int64 total_data_size_bytes = 4;
+ // deleted_data_size_bytes is the size of data that has been deleted in
bytes.
+ int64 deleted_data_size_bytes = 5;
+ // message provides additional information about the task status.
+ string message = 6;
+ // created_at is the timestamp when the task was created.
+ google.protobuf.Timestamp created_at = 7;
+}
+
+// GroupRegistryServiceQueryRequest is the request for querying a group
deletion task.
+message GroupRegistryServiceQueryRequest {
+ // group is the name of the group whose deletion task to query.
+ string group = 1;
+}
+
+// GroupRegistryServiceQueryResponse is the response for querying a group
deletion task.
+message GroupRegistryServiceQueryResponse {
+ // task contains the status of the deletion task.
+ GroupDeletionTask task = 1;
}
Review Comment:
The GroupDeletionTask message and the Query/Inspect RPCs are defined in the
API but have no corresponding implementation. Searching the codebase shows no
implementation for GroupRegistryServiceQueryRequest or
GroupRegistryServiceInspectRequest. This means these new endpoints will return
"Unimplemented" errors when called.
```suggestion
```
##########
bydbctl/internal/cmd/group_test.go:
##########
@@ -95,15 +95,6 @@ resource_opts:
Expect(out).To(ContainSubstring("group group1 is updated"))
})
- It("delete group", func() {
- rootCmd.SetArgs([]string{"group", "delete", "-g", "group1"})
- out := capturer.CaptureStdout(func() {
- err := rootCmd.Execute()
- Expect(err).NotTo(HaveOccurred())
- })
- Expect(out).To(ContainSubstring("group group1 is deleted"))
- })
-
It("list group", func() {
Review Comment:
The test for group deletion was removed without adding a replacement test.
Since the Delete API has been significantly enhanced with new parameters
(dry_run, force) and new response fields (task_id), comprehensive tests should
be added to verify these new features work correctly. The removal of existing
test coverage without replacement reduces test quality.
--
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]
