Copilot commented on code in PR #1115:
URL:
https://github.com/apache/skywalking-banyandb/pull/1115#discussion_r3194128955
##########
docs/api-reference.md:
##########
@@ -5282,6 +5282,7 @@ Phase represents the current phase of the deletion task.
| catalog | [string](#string) | | |
| resource_opts |
[banyandb.common.v1.ResourceOpts](#banyandb-common-v1-ResourceOpts) | | |
| data_info | [banyandb.database.v1.DataInfo](#banyandb-database-v1-DataInfo)
| repeated | |
+| errors | [string](#string) | repeated | errors lists every failure observed
while collecting data_info for this group: top-level CollectDataInfo failures
(GetGroup, missing collector, dial failure -- prefixed "top-level: ")
and per-node broadcast failures (prefixed "future error: ", "node
error: ", "broadcast failed: "). Combined with len(data_info),
consumers can tell three states apart: data_info empty && errors empty
-> no nodes reported (group inactive) data_info empty && errors
non-empty -> total failure data_info non-empty && errors non-empty
-> partial failure data_info non-empty && errors empty -> full
success |
Review Comment:
This field description says consumers can tell “three states apart”, but it
lists four distinct states. Please adjust the wording for consistency (e.g.,
“four states” or “the following states”), and consider formatting as separate
bullet points for readability.
##########
api/proto/banyandb/fodc/v1/rpc.proto:
##########
@@ -114,6 +114,16 @@ message GroupLifecycleInfo {
string catalog = 2;
banyandb.common.v1.ResourceOpts resource_opts = 3;
repeated banyandb.database.v1.DataInfo data_info = 4;
+ // errors lists every failure observed while collecting data_info for this
+ // group: top-level CollectDataInfo failures (GetGroup, missing collector,
+ // dial failure -- prefixed "top-level: ") and per-node broadcast failures
+ // (prefixed "future error: ", "node error: ", "broadcast failed: ").
+ // Combined with len(data_info), consumers can tell three states apart:
+ // data_info empty && errors empty -> no nodes reported (group inactive)
+ // data_info empty && errors non-empty -> total failure
+ // data_info non-empty && errors non-empty -> partial failure
+ // data_info non-empty && errors empty -> full success
Review Comment:
The comment says consumers can tell “three states apart”, but the list
enumerates four distinct states (inactive/total failure/partial failure/full
success). Please update the wording to match (e.g., “four states” or “these
states”).
##########
banyand/metadata/schema/collector.go:
##########
@@ -72,22 +80,27 @@ func NewInfoCollectorRegistry(l *logger.Logger, groupGetter
GroupGetter) *InfoCo
}
}
-// CollectDataInfo collects data information from both local and remote data
nodes.
-func (icr *InfoCollectorRegistry) CollectDataInfo(ctx context.Context, group
string) ([]*databasev1.DataInfo, error) {
+// CollectDataInfo collects data information from both local and remote data
+// nodes. Returns the aggregated DataInfo slice plus a parallel slice of
+// per-node collection errors observed during broadcast (broadcast failure,
+// future errors, listener-side common.Error responses). The error return
+// is reserved for top-level failures that prevent any aggregation
+// (GetGroup failure, local collector failure, unsupported catalog).
Review Comment:
The doc comment says CollectDataInfo returns a “parallel” slice of per-node
collection errors, but the error slice is not 1:1 aligned with the returned
DataInfo slice (it only contains failures). Consider rewording to “separate
slice of per-node collection errors” to avoid implying positional
correspondence.
--
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]
