Copilot commented on code in PR #927:
URL:
https://github.com/apache/skywalking-banyandb/pull/927#discussion_r2666521634
##########
banyand/queue/sub/server.go:
##########
@@ -283,6 +289,7 @@ func (s *server) Serve() run.StopNotify {
grpc_health_v1.RegisterHealthServer(s.ser, health.NewServer())
databasev1.RegisterSnapshotServiceServer(s.ser, s)
databasev1.RegisterNodeQueryServiceServer(s.ser, s)
+ databasev1.RegisterClusterStateServiceServer(s.ser, s)
Review Comment:
The server registers ClusterStateService (line 292) but does not implement
the GetClusterState method. When schemaRegistry is nil (as in the test case at
line 81 in banyand/queue/test/chunked_sync_common.go), any calls to
GetClusterState will use the default unimplemented method from
UnimplementedClusterStateServiceServer, which returns an "Unimplemented" error.
If this service is intended to be available on data nodes, the
GetClusterState method should be implemented similar to the liaison server
implementation. If it's not needed for data nodes, the service should not be
registered.
```suggestion
if s.schemaRegistry != nil {
databasev1.RegisterClusterStateServiceServer(s.ser, s)
}
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -684,3 +684,15 @@ message GetCurrentNodeResponse {
service NodeQueryService {
rpc GetCurrentNode(GetCurrentNodeRequest) returns (GetCurrentNodeResponse) {}
}
+
+message GetClusterStateRequest {}
+
+message GetClusterStateResponse {
Review Comment:
The GetClusterStateResponse message documentation is incomplete. The 'nodes'
field should have a description explaining what nodes are returned (e.g.,
"nodes contains all discovered nodes in the cluster across all roles").
```suggestion
message GetClusterStateResponse {
// nodes contains all discovered nodes in the cluster across all roles.
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -684,3 +684,15 @@ message GetCurrentNodeResponse {
service NodeQueryService {
rpc GetCurrentNode(GetCurrentNodeRequest) returns (GetCurrentNodeResponse) {}
}
+
+message GetClusterStateRequest {}
+
+message GetClusterStateResponse {
+ repeated banyandb.database.v1.Node nodes = 1;
+}
+
+service ClusterStateService {
+ rpc GetClusterState(GetClusterStateRequest) returns
(GetClusterStateResponse) {
+ option (google.api.http) = {get: "/v1/cluster/state"};
+ }
+}
Review Comment:
The ClusterStateService and GetClusterState RPC should have documentation
comments explaining their purpose. For example: "ClusterStateService provides
information about the current state of the cluster" and "GetClusterState
returns all nodes discovered in the cluster across liaison and data nodes".
##########
banyand/queue/sub/server.go:
##########
@@ -88,6 +91,7 @@ type server struct {
httpSrv *http.Server
curNode *databasev1.Node
clientCloser context.CancelFunc
+ clusterStateService databasev1.ClusterStateServiceServer
Review Comment:
The field clusterStateService is declared but never initialized or used. It
appears to be leftover from development. This field should be removed as it
serves no purpose and creates confusion with the ClusterStateService
functionality.
```suggestion
```
##########
api/proto/banyandb/database/v1/rpc.proto:
##########
@@ -684,3 +684,15 @@ message GetCurrentNodeResponse {
service NodeQueryService {
rpc GetCurrentNode(GetCurrentNodeRequest) returns (GetCurrentNodeResponse) {}
}
+
Review Comment:
The GetClusterStateRequest message documentation is empty. Consider adding a
description explaining the purpose of this request, even if it has no fields.
For example: "GetClusterStateRequest is an empty request message for retrieving
the current state of all nodes in the cluster."
```suggestion
// GetClusterStateRequest is an empty request message for retrieving the
// current state of all nodes in the cluster.
```
##########
banyand/queue/sub/server.go:
##########
@@ -308,6 +315,13 @@ func (s *server) Serve() run.StopNotify {
close(stopCh)
return stopCh
}
+ if s.clusterStateService != nil {
+ if err :=
databasev1.RegisterClusterStateServiceHandlerFromEndpoint(ctx, gwMux, s.addr,
clientOpts); err != nil {
+ s.log.Error().Err(err).Msg("Failed to register cluster
state service")
+ close(stopCh)
+ return stopCh
+ }
+ }
Review Comment:
The conditional check for s.clusterStateService != nil will always be true
(checking if the field is nil), but the field is never initialized in the
server struct. This field appears to be unused and the check is likely
incorrect.
If the intention is to conditionally register the HTTP handler based on
whether schemaRegistry is available, the condition should check
s.schemaRegistry != nil instead. Additionally, since GetClusterState is not
implemented in this server, this handler registration will fail or return
unimplemented errors.
--
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]
