Copilot commented on code in PR #913:
URL:
https://github.com/apache/skywalking-banyandb/pull/913#discussion_r2640400750
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -354,10 +355,56 @@ message GroupRegistryServiceUpdateResponse {}
message GroupRegistryServiceDeleteRequest {
string group = 1;
+ bool dry_run = 2;
+ bool force = 3;
}
message GroupRegistryServiceDeleteResponse {
bool deleted = 1;
Review Comment:
The GroupRegistryServiceDeleteResponse has both 'deleted' and 'task_id'
fields. Consider documenting the relationship between these fields. For example:
- When is 'deleted' true vs false?
- Is 'task_id' only populated for async operations (when force=false)?
- Is 'task_id' empty for dry_run operations?
This clarification would help API consumers understand when to poll for task
status vs. treat the operation as complete
```suggestion
string group = 1;
// If true, the server MUST NOT perform any destructive action.
// Instead, it validates the delete request and returns an estimate of the
// impact via related progress/inspection APIs.
//
// In dry-run mode, GroupRegistryServiceDeleteResponse.deleted is expected
// to be false and no deletion task should be created, so
// GroupRegistryServiceDeleteResponse.task_id is typically empty.
bool dry_run = 2;
// If true, the server SHOULD attempt to delete the group and its data
// synchronously, blocking until the operation either succeeds or fails.
//
// When a force delete succeeds synchronously, the server is expected to
// return GroupRegistryServiceDeleteResponse.deleted = true and may leave
// GroupRegistryServiceDeleteResponse.task_id empty.
//
// When false, the server MAY schedule deletion asynchronously. In that
// case, GroupRegistryServiceDeleteResponse.deleted will be false and
// GroupRegistryServiceDeleteResponse.task_id will contain the identifier
// of the background deletion task.
bool force = 3;
}
// Response for a group delete operation.
//
// The relationship between the fields is as follows:
// * deleted == true:
// The group has been fully deleted by the time this response is
// returned. Implementations typically set this when the deletion
// completed synchronously (for example, for successful force deletes).
// In this case, task_id is usually empty because there is no
// background task to monitor.
// * deleted == false:
// The group has not been fully deleted. This can happen when:
// - the operation is a dry run (no deletion performed),
// - the deletion is scheduled or running asynchronously, or
// - the deletion failed to start.
// For asynchronous deletions, task_id is populated so clients can poll
// a progress endpoint (e.g., via GroupDeletionProgress) to monitor
// the background task.
//
// For dry_run requests, servers are expected to leave deleted as false and
// not create any background task. In that case task_id should be empty.
message GroupRegistryServiceDeleteResponse {
// Whether the group has been fully deleted at the time of this response.
bool deleted = 1;
// Identifier of an asynchronous deletion task, when applicable.
//
// This is typically set when the delete operation is handled
// asynchronously (for example, when force == false) and remains empty
// when the deletion finished synchronously or when the request was a
// dry run.
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -354,10 +355,56 @@ message GroupRegistryServiceUpdateResponse {}
message GroupRegistryServiceDeleteRequest {
string group = 1;
+ bool dry_run = 2;
+ bool force = 3;
}
message GroupRegistryServiceDeleteResponse {
bool deleted = 1;
Review Comment:
Add a documentation comment for the task_id field to explain when it's
populated (e.g., only for async deletion operations, empty for
immediate/dry-run deletions)
```suggestion
bool deleted = 1;
// Identifier of the asynchronous deletion task. This is populated only
when
// the group deletion is performed asynchronously, and is empty for
immediate
// deletions or when the request is a dry run.
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -354,10 +355,56 @@ message GroupRegistryServiceUpdateResponse {}
message GroupRegistryServiceDeleteRequest {
string group = 1;
+ bool dry_run = 2;
+ bool force = 3;
}
message GroupRegistryServiceDeleteResponse {
bool deleted = 1;
+ string task_id = 2;
+}
+
+message GroupDeletionProgress {
+ enum Phase {
+ PHASE_UNSPECIFIED = 0;
+ PHASE_PENDING = 1;
+ PHASE_IN_PROGRESS = 2;
+ PHASE_COMPLETED = 3;
+ PHASE_FAILED = 4;
+ }
+ Phase current_phase = 1;
+ int32 stream_count = 2;
+ int32 measure_count = 3;
+ int32 trace_count = 4;
+ int64 data_size_bytes = 5;
+ string message = 6;
+ bool is_dry_run = 7;
+ int64 deleted_data_bytes = 8;
Review Comment:
All fields in the GroupDeletionProgress message lack documentation comments.
Add comments to explain the purpose of each field, especially:
- stream_count, measure_count, trace_count: Are these total counts or
deleted counts?
- data_size_bytes: Is this the total size or remaining size?
- deleted_data_bytes: Should clarify relationship with data_size_bytes
- message vs error_message: Clarify when each should be used
```suggestion
}
// The current lifecycle phase of the group deletion task.
Phase current_phase = 1;
// Total number of stream entities that are targeted to be deleted in this
task.
// This is the overall count, not the number already deleted.
int32 stream_count = 2;
// Total number of measure entities that are targeted to be deleted in
this task.
// This is the overall count, not the number already deleted.
int32 measure_count = 3;
// Total number of trace entities that are targeted to be deleted in this
task.
// This is the overall count, not the number already deleted.
int32 trace_count = 4;
// Total size, in bytes, of all data that is planned or estimated to be
deleted
// as part of this task. This represents the full data size, not the
remaining size.
int64 data_size_bytes = 5;
// Human-readable status or progress message for this task (for example,
// describing the current phase or high-level progress). This is used for
// non-error informational messages and is typically populated in pending,
// in-progress, or completed phases.
string message = 6;
// Indicates whether this deletion task is a dry run. When true, no data is
// actually removed and the size and count fields represent what would be
// deleted if the task were executed for real.
bool is_dry_run = 7;
// Total size, in bytes, of data that has already been deleted successfully
// by this task so far. Together with data_size_bytes, this can be used to
// derive the remaining size to delete (data_size_bytes -
deleted_data_bytes).
int64 deleted_data_bytes = 8;
// Human-readable description of the error that caused the task to fail.
// This field should be populated when current_phase is PHASE_FAILED and is
// intended for diagnostics, while "message" is reserved for general
status.
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -354,10 +355,56 @@ message GroupRegistryServiceUpdateResponse {}
message GroupRegistryServiceDeleteRequest {
string group = 1;
+ bool dry_run = 2;
+ bool force = 3;
}
message GroupRegistryServiceDeleteResponse {
bool deleted = 1;
+ string task_id = 2;
+}
+
+message GroupDeletionProgress {
+ enum Phase {
+ PHASE_UNSPECIFIED = 0;
+ PHASE_PENDING = 1;
+ PHASE_IN_PROGRESS = 2;
+ PHASE_COMPLETED = 3;
+ PHASE_FAILED = 4;
+ }
+ Phase current_phase = 1;
+ int32 stream_count = 2;
+ int32 measure_count = 3;
+ int32 trace_count = 4;
+ int64 data_size_bytes = 5;
+ string message = 6;
+ bool is_dry_run = 7;
+ int64 deleted_data_bytes = 8;
+ string error_message = 9;
+}
+
+message GroupDeletionTask {
+ string task_id = 1;
+ string group_name = 2;
+ GroupDeletionProgress progress = 3;
+ google.protobuf.Timestamp created_at = 4;
+}
+
+message GroupDeletionTaskGetRequest {
+ string task_id = 1;
+}
+
+message GroupDeletionTaskGetResponse {
+ GroupDeletionTask task = 1;
+}
+
+message GroupDeletionTaskListRequest {
+ string group_name = 1;
Review Comment:
Add documentation comments for the include_completed field to clarify
whether this includes only completed tasks, or both completed and non-completed
tasks when set to true
```suggestion
string group_name = 1;
// When true, the response includes both non-completed and completed tasks
for the given group.
// When false, only non-completed (e.g., pending or in-progress) tasks are
returned.
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -354,10 +355,56 @@ message GroupRegistryServiceUpdateResponse {}
message GroupRegistryServiceDeleteRequest {
string group = 1;
+ bool dry_run = 2;
+ bool force = 3;
}
message GroupRegistryServiceDeleteResponse {
bool deleted = 1;
+ string task_id = 2;
+}
+
+message GroupDeletionProgress {
+ enum Phase {
+ PHASE_UNSPECIFIED = 0;
+ PHASE_PENDING = 1;
+ PHASE_IN_PROGRESS = 2;
+ PHASE_COMPLETED = 3;
+ PHASE_FAILED = 4;
+ }
+ Phase current_phase = 1;
+ int32 stream_count = 2;
+ int32 measure_count = 3;
+ int32 trace_count = 4;
+ int64 data_size_bytes = 5;
+ string message = 6;
+ bool is_dry_run = 7;
+ int64 deleted_data_bytes = 8;
+ string error_message = 9;
+}
+
+message GroupDeletionTask {
+ string task_id = 1;
+ string group_name = 2;
+ GroupDeletionProgress progress = 3;
Review Comment:
Add documentation comments for the GroupDeletionTask message fields to
explain:
- task_id: Format and how it's generated
- group_name: The name of the group being deleted
- progress: Current progress of the deletion task
- created_at: When the deletion task was created
```suggestion
message GroupDeletionTask {
// Unique identifier for this deletion task.
// This value is assigned by the server and should be treated as an opaque
string
// with no guaranteed format; clients must not rely on any particular
structure.
string task_id = 1;
// Name of the group that is being deleted by this task.
string group_name = 2;
// Current progress information and status of the group deletion operation.
GroupDeletionProgress progress = 3;
// Timestamp indicating when this deletion task was created on the server.
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -354,10 +355,56 @@ message GroupRegistryServiceUpdateResponse {}
message GroupRegistryServiceDeleteRequest {
string group = 1;
+ bool dry_run = 2;
+ bool force = 3;
}
message GroupRegistryServiceDeleteResponse {
bool deleted = 1;
+ string task_id = 2;
+}
+
+message GroupDeletionProgress {
+ enum Phase {
+ PHASE_UNSPECIFIED = 0;
+ PHASE_PENDING = 1;
+ PHASE_IN_PROGRESS = 2;
+ PHASE_COMPLETED = 3;
+ PHASE_FAILED = 4;
+ }
+ Phase current_phase = 1;
+ int32 stream_count = 2;
+ int32 measure_count = 3;
+ int32 trace_count = 4;
+ int64 data_size_bytes = 5;
+ string message = 6;
Review Comment:
The GroupDeletionProgress message has both a 'message' field (line 380) and
an 'error_message' field (line 383). Consider renaming 'message' to
'status_message' or 'info_message' to clearly distinguish it from error
messages and avoid ambiguity
```suggestion
string status_message = 6;
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -411,6 +458,14 @@ service GroupRegistryService {
// Exist doesn't expose an HTTP endpoint. Please use HEAD method to touch
Get instead
rpc Exist(GroupRegistryServiceExistRequest) returns
(GroupRegistryServiceExistResponse);
+
+ rpc GetDeletionTask(GroupDeletionTaskGetRequest) returns
(GroupDeletionTaskGetResponse) {
+ option (google.api.http) = {get: "/v1/group/deletion-tasks/{task_id}"};
+ }
+
Review Comment:
The ListDeletionTasks HTTP endpoint doesn't specify how query parameters are
mapped. Since GroupDeletionTaskListRequest has 'group_name' and
'include_completed' fields, these will be passed as query parameters. Consider
explicitly documenting the expected URL format (e.g.,
/v1/group/deletion-tasks?group_name=xxx&include_completed=true) in a comment
for API consumers
```suggestion
// ListDeletionTasks is exposed via HTTP GET.
// Query parameters are mapped from GroupDeletionTaskListRequest fields,
e.g.:
// GET
/v1/group/deletion-tasks?group_name=<group_name>&include_completed=<true|false>
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -354,10 +355,56 @@ message GroupRegistryServiceUpdateResponse {}
message GroupRegistryServiceDeleteRequest {
string group = 1;
+ bool dry_run = 2;
Review Comment:
Add documentation comments for the new dry_run and force fields to explain:
- dry_run: Whether to simulate the deletion without actually deleting data
- force: Whether to force deletion even if the group contains resources
```suggestion
string group = 1;
// Whether to simulate the deletion without actually deleting data.
bool dry_run = 2;
// Whether to force deletion even if the group contains resources.
```
--
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]
