You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@skywalking.apache.org by "hanahmily (via GitHub)" <gi...@apache.org> on 2026/04/26 03:26:08 UTC
[PR] Schema consistency Phase 1: revision gates and barrier RPCs [skywalking-banyandb]
hanahmily opened a new pull request, #1091:
URL: https://github.com/apache/skywalking-banyandb/pull/1091
## Summary
Phase 1 of the schema-consistency protocol — gives clients an observable, opt-in way to align writes and queries with schema-propagation state in standalone deployments.
- **Response surface** — `mod_revision` on Group / IndexRule / IndexRuleBinding / TopNAggregation Create/Update responses; `delete_time` on every `*ServiceDeleteResponse`; `created_at` on Stream / Measure / Trace / Property / IndexRule / IndexRuleBinding / TopNAggregation / Group (preserved across updates).
- **Write-path gate** — Stream and Measure write RPCs now apply a three-way `mod_revision` split (`<` `STATUS_EXPIRED_SCHEMA`, `==` `STATUS_SUCCEED`, `>` held then `STATUS_SCHEMA_NOT_APPLIED`, `0` skipped). `STATUS_SCHEMA_NOT_APPLIED` (10) is the only new enum value.
- **Query-path gate** — Stream / Measure / Trace `QueryRequest.group_mod_revisions` (per-group `map<string, int64>`) with per-group outcomes in `QueryResponse.group_statuses`. Empty map preserves prior behavior.
- **Time-range clamp** — queries whose `time_range.start` falls before a schema's `created_at` are clamped to `max(start, created_at)`; multi-group uses the max across queried groups; nil for pre-upgrade schemas is a no-op.
- **`SchemaBarrierService`** — `AwaitRevisionApplied`, `AwaitSchemaApplied`, `AwaitSchemaDeleted`. 10 000-key cap, timeout-bounded, returns laggards on expiry.
- **Tombstone retention/GC** — default 7d (configurable via `--schema-server-tombstone-retention`, 1h floor) with a per-cache count cap to bound memory under bulk deletes.
- **Re-create invariant** — `Create` with `updated_at <= tombstone.delete_time` is rejected with `InvalidArgument: updated_at_before_tombstone`.
- **Stale-delete guard** — `pkg/schema/cache` drops out-of-order `EventDelete` events; exposes monotonic `LatestModRevision` watermark.
All gates are opt-in; clients sending zero-valued `mod_revision` and empty `group_mod_revisions` keep the prior behavior unchanged.
## Acceptance evidence
A1–A13 each trace to ≥1 named test (matrix in the implementation plan). Local CI is green:
- `make license-check && make check-req && make build && make lint && make check` — clean
- Unit tests — `./banyand/...`, `./bydbctl/...`, `./pkg/...`, `./fodc/...` all pass
- Integration tests — `./test/integration/standalone/...`, `./test/integration/distributed/...` all pass
- OAP-mirror suite (§6.1–§6.11) plus §4.4–§4.7 (write/query gates, time-range clamp, tombstone GC) — 28/28 specs green
## Documentation
- `CHANGES.md` — schema-consistency feature cluster added under `## 0.11.0` → `### Features`
- `docs/interacting/schema-consistency/` — four new pages: overview, barrier RPCs, revision gates, scenarios
- `docs/menu.yml` — wired into the Interacting catalog
## Test plan
- [ ] CI: license-check, check-req, build, lint, check all green
- [ ] CI: unit tests pass for `./banyand/...`, `./bydbctl/...`, `./pkg/...`, `./fodc/...`
- [ ] CI: integration tests pass for `./test/integration/standalone/...` and `./test/integration/distributed/...`
- [ ] Reviewer: confirm `STATUS_SCHEMA_NOT_APPLIED = 10` does not collide with any downstream client fork
- [ ] Reviewer: confirm proto field tags (50, 51 on query messages; 1–3 on delete responses) do not clash with reserved ranges
- [ ] Reviewer: confirm `created_at` on the eight schema messages is additive
- [ ] Reviewer: cross-language wire compatibility check for the new `map<string, ...>` fields on `QueryRequest`/`QueryResponse`
- [ ] OAP client maintainer: dry-run against the new surface; treat `STATUS_SCHEMA_NOT_APPLIED` as `STATUS_EXPIRED_SCHEMA` until the Java client enum is updated
## Out of scope (Phase 2, v0.13.0)
Cluster-side fan-out for the barrier RPCs (`NodeSchemaStatusService`, liaison aggregation, laggard reporting) and metrics/access-logging for the new barriers ship in Phase 2.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema consistency Phase 1: revision gates and barrier RPCs [skywalking-banyandb]
Posted by "hanahmily (via GitHub)" <gi...@apache.org>.
hanahmily merged PR #1091:
URL: https://github.com/apache/skywalking-banyandb/pull/1091
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Schema consistency Phase 1: revision gates and barrier RPCs [skywalking-banyandb]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #1091:
URL: https://github.com/apache/skywalking-banyandb/pull/1091#discussion_r3142929055
##########
banyand/liaison/grpc/stream.go:
##########
@@ -386,6 +403,37 @@ func (s *streamService) Query(ctx context.Context, req *streamv1.QueryRequest) (
}
}()
}
+ gatedStatuses, shortCircuit := checkQueryGate(req.Groups, req.Name, req.GroupModRevisions,
+ func(name, group string) (int64, bool) {
+ loc, ok := s.entityRepo.getLocator(identity{name: name, group: group})
+ return loc.ModRevision, ok
+ }, s.maxWaitDuration)
+ if shortCircuit {
+ return &streamv1.QueryResponse{GroupStatuses: gatedStatuses}, nil
+ }
+ // See measure.go for rationale: gate the clamp on the same opt-in trigger as the
+ // query gate so legacy clients that omit GroupModRevisions are not affected.
+ if req.TimeRange != nil && len(req.GroupModRevisions) > 0 {
+ createdAts := make([]time.Time, 0, len(req.Groups))
+ for _, group := range req.Groups {
+ streamEntity, streamOK := s.entityRepo.getStream(identity{name: req.Name, group: group})
+ if streamOK && streamEntity.GetCreatedAt() != nil {
+ createdAts = append(createdAts, streamEntity.GetCreatedAt().AsTime())
+ }
+ }
+ endTime := time.Time{}
+ if req.TimeRange.GetEnd() != nil {
+ endTime = req.TimeRange.GetEnd().AsTime()
+ }
+ originalBegin := req.TimeRange.GetBegin().AsTime()
+ newBegin, rangeEmpty := clampTimeRangeBegin(originalBegin, endTime, createdAts)
+ if rangeEmpty {
Review Comment:
When clamping makes the time range empty, this returns `emptyStreamQueryResponse` without `group_statuses`. If the request included `group_mod_revisions`, clients may expect `group_statuses` to be present (at least for the gated groups that succeeded). Consider returning an empty response that still includes `GroupStatuses: gatedStatuses` when `len(gatedStatuses) > 0`.
```suggestion
if rangeEmpty {
if len(gatedStatuses) > 0 {
return &streamv1.QueryResponse{GroupStatuses: gatedStatuses}, nil
}
```
##########
CHANGES.md:
##########
@@ -19,6 +19,18 @@ Release Notes.
- The `--namespace` CLI flag has been removed (it previously configured the etcd key prefix).
- The `--node-discovery-mode` flag no longer accepts `etcd` (supported values: `none`, `dns`, `file`).
- The `--schema-registry-mode` flag only accepts `property`.
+- Schema consistency (Phase 1): introduce client-observable revision and propagation primitives. All gates are opt-in and zero-valued requests preserve prior behavior.
+ - Add `mod_revision` to Group / IndexRule / IndexRuleBinding / TopNAggregation Create and Update responses.
+ - Add `delete_time` to all `*ServiceDeleteResponse` messages so clients can observe tombstones.
+ - Add `created_at` to Stream / Measure / Trace / Property / IndexRule / IndexRuleBinding / TopNAggregation / Group; preserved across updates.
+ - Add `STATUS_SCHEMA_NOT_APPLIED` (10) for writes/queries whose `mod_revision` is ahead of the server cache.
+ - Add three-way write-path `ModRevision` gate on Stream and Measure write RPCs (`<` expired, `==` succeed, `>` not-applied, `0` skipped).
+ - Add per-group query-path gate via `QueryRequest.group_mod_revisions` and `QueryResponse.group_statuses` for Stream / Measure / Trace queries.
+ - Add automatic time-range clamp `max(time_range.start, schema.created_at)`; multi-group uses the maximum across queried groups; nil for pre-upgrade schemas is a no-op.
+ - Add `SchemaBarrierService` with `AwaitRevisionApplied`, `AwaitSchemaApplied`, `AwaitSchemaDeleted` (10 000-key cap, timeout-bounded, returns laggards on expiry).
+ - Add tombstone retention/GC (default 7 days, 1 hour floor, configurable via `--schema-server-tombstone-retention`) with a per-cache count cap to bound memory under bulk deletes.
Review Comment:
This bullet claims a "1 hour floor" for `--schema-server-tombstone-retention`, but the current schema-server validation accepts short values (needed by integration tests). Please align this release-note statement with the actual behavior (either reintroduce the floor or update this text).
##########
pkg/schema/cache.go:
##########
@@ -87,22 +90,101 @@ type schemaRepo struct {
indexRuleMap sync.Map
bindingForwardMap sync.Map
bindingBackwardMap sync.Map
+ latestModRevision atomic.Int64
workerNum int
resourceMutex sync.Mutex
groupMux sync.Mutex
}
+// SendMetadataEvent applies the given event synchronously. When the downstream
+// caches (e.g. pkg/schema.schemaRepo.groupMap and resourceMap) must be coherent
+// before the caller returns — for example, when SchemaBarrierService delegates
+// through notifyHandlers → this handler → SendMetadataEvent — the caller relies
+// on the event being fully applied here. Only transient errors queue the event
+// for retry via the background worker; the success path does not touch the
+// channel.
func (sr *schemaRepo) SendMetadataEvent(event MetadataEvent) {
if !sr.closer.AddSender() {
return
}
defer sr.closer.SenderDone()
- select {
- case sr.eventCh <- event:
- case <-sr.closer.CloseNotify():
+ if err := sr.processEvent(event); err != nil && !errors.Is(err, schema.ErrClosed) {
+ select {
+ case <-sr.closer.CloseNotify():
+ return
+ default:
+ }
+ sr.l.Err(err).Interface("event", event).Msg("fail to handle the metadata event. retry...")
+ sr.metrics.totalErrs.Inc(1)
+ select {
+ case sr.eventCh <- event:
+ case <-sr.closer.CloseNotify():
+ }
}
}
+// processEvent applies the event in-place, bumping latestModRevision on success
+// so the barrier's downstream-handler watermark catches up before notifyHandlers
+// returns to its caller.
+func (sr *schemaRepo) processEvent(evt MetadataEvent) error {
+ if e := sr.l.Debug(); e.Enabled() {
+ e.Interface("event", evt).Msg("received an event")
+ }
+ var err error
+ switch evt.Typ {
+ case EventAddOrUpdate:
+ switch evt.Kind {
+ case EventKindGroup:
+ _, err = sr.storeGroup(evt.Metadata.GetMetadata())
+ if errors.As(err, schema.ErrGRPCResourceNotFound) {
+ err = nil
+ }
Review Comment:
`errors.As(err, schema.ErrGRPCResourceNotFound)` is incorrect here: ErrGRPCResourceNotFound is an `error` value, but `errors.As` requires a pointer target and will panic when `err != nil`. Use `errors.Is(err, schema.ErrGRPCResourceNotFound)` instead (matching the pattern used elsewhere in the repo).
##########
test/cases/schema/tombstone_gc.go:
##########
@@ -0,0 +1,186 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+// Package schema contains shared test cases for schema-related functionality.
+//
+// §4.7.1 and §4.7.2 exercise tombstone retention behavior. The test suites wire
+// the standalone server with --schema-server-tombstone-retention=2s in the
+// BeforeSuite, so the specs run without requiring any environment variable.
+package schema
+
+import (
+ "context"
+ "fmt"
+ "time"
+
+ g "github.com/onsi/ginkgo/v2"
+ gm "github.com/onsi/gomega"
+
+ commonv1 "github.com/apache/skywalking-banyandb/api/proto/banyandb/common/v1"
+ databasev1 "github.com/apache/skywalking-banyandb/api/proto/banyandb/database/v1"
+)
+
+// tombstoneRetentionForTest mirrors the --schema-server-tombstone-retention flag
+// configured in the integration suite setup. Keep in sync with common.go.
+const tombstoneRetentionForTest = 2 * time.Second
+
+// Schema tombstone GC specs — §4.7.1 / §4.7.2.
+// These specs require a short tombstone_retention (e.g. 2s) to be configured.
+// Set BANYANDB_TOMBSTONE_RETENTION=2s before running the suite.
Review Comment:
The header comment says the suite wiring removes the need for environment variables, but this line still instructs setting `BANYANDB_TOMBSTONE_RETENTION`. Please drop or update this line to avoid confusing contributors running the suite.
##########
banyand/liaison/grpc/barrier.go:
##########
@@ -0,0 +1,255 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package grpc
+
+import (
+ "context"
+ "time"
+
+ "google.golang.org/grpc/codes"
+ "google.golang.org/grpc/status"
+ "google.golang.org/protobuf/types/known/durationpb"
+
+ schemav1 "github.com/apache/skywalking-banyandb/api/proto/banyandb/schema/v1"
+ "github.com/apache/skywalking-banyandb/banyand/metadata/schema"
+)
+
+const (
+ barrierMaxKeys = 10000
+ barrierInitInterval = 10 * time.Millisecond
+ barrierMaxInterval = 500 * time.Millisecond
+ barrierGrowthFactor = 1.5
+ // barrierDefaultTimeout bounds calls that pass a nil/zero timeout. It must
+ // be large enough to cover normal schema propagation (watch event delivery
+ // + handler dispatch + tsdb open) so a no-timeout caller does not see false
+ // applied=false. Distinct from barrierMaxInterval, which only caps the poll
+ // backoff between probes.
+ barrierDefaultTimeout = 5 * time.Second
+)
+
+// barrierCacheReader abstracts the property schema cache so the barrier service
+// can poll for mod-revision progress without coupling to a concrete type.
+type barrierCacheReader interface {
+ GetMaxModRevision() int64
+ GetKeyModRevision(propID string) (int64, bool)
+}
+
+type barrierService struct {
+ schemav1.UnimplementedSchemaBarrierServiceServer
+ cacheProvider func() barrierCacheReader
+}
+
+func newBarrierService(cacheProvider func() barrierCacheReader) *barrierService {
+ return &barrierService{cacheProvider: cacheProvider}
+}
+
+// cache resolves the current barrier cache reader. It returns nil if the
+// underlying schema registry is not yet initialized.
+func (b *barrierService) cache() barrierCacheReader {
+ if b.cacheProvider == nil {
+ return nil
+ }
+ return b.cacheProvider()
+}
+
+// AwaitRevisionApplied blocks until the cache's max modRevision is >= req.MinRevision
+// or the timeout elapses. In standalone mode there is only one node, so Laggards is always empty.
+func (b *barrierService) AwaitRevisionApplied(ctx context.Context, req *schemav1.AwaitRevisionAppliedRequest) (*schemav1.AwaitRevisionAppliedResponse, error) {
+ deadline := time.Now().Add(barrierDeadlineDuration(req.GetTimeout()))
+ pollCtx, cancel := context.WithDeadline(ctx, deadline)
+ defer cancel()
+ interval := barrierInitInterval
+ for {
+ if c := b.cache(); c != nil && c.GetMaxModRevision() >= req.GetMinRevision() {
+ return &schemav1.AwaitRevisionAppliedResponse{Applied: true}, nil
+ }
+ if time.Now().After(deadline) {
+ return &schemav1.AwaitRevisionAppliedResponse{Applied: false}, nil
+ }
+ select {
+ case <-time.After(interval):
+ case <-pollCtx.Done():
+ return &schemav1.AwaitRevisionAppliedResponse{Applied: false}, nil
+ }
Review Comment:
On timeout/cancellation, `AwaitRevisionApplied` returns `applied=false` but leaves `laggards` empty. The docs/PR description indicate the barriers should return laggards on expiry; even in standalone mode it would be useful to return a single `NodeLaggard` with `current_mod_revision` set to the cache watermark so clients can diagnose how far behind they are.
##########
banyand/liaison/grpc/measure.go:
##########
@@ -419,6 +436,41 @@ func (ms *measureService) Query(ctx context.Context, req *measurev1.QueryRequest
}
}()
}
+ gatedStatuses, shortCircuit := checkQueryGate(req.Groups, req.Name, req.GroupModRevisions,
+ func(name, group string) (int64, bool) {
+ loc, ok := ms.entityRepo.getLocator(identity{name: name, group: group})
+ return loc.ModRevision, ok
+ }, ms.maxWaitDuration)
+ if shortCircuit {
+ return &measurev1.QueryResponse{GroupStatuses: gatedStatuses}, nil
+ }
+ // Gate the clamp on the same opt-in trigger as the query gate (GroupModRevisions
+ // non-empty). Schema-aware clients that pass per-group revisions also accept
+ // pre-creation filtering; legacy clients that omit GroupModRevisions retain the
+ // pre-Step-1.6 behavior where data with timestamps preceding CreatedAt is still
+ // returned. This keeps backward compatibility for fixtures that write historical
+ // data into a freshly-created measure.
+ if req.TimeRange != nil && len(req.GroupModRevisions) > 0 {
+ createdAts := make([]time.Time, 0, len(req.Groups))
+ for _, group := range req.Groups {
+ measureEntity, measureOK := ms.entityRepo.getMeasure(identity{name: req.Name, group: group})
+ if measureOK && measureEntity.GetCreatedAt() != nil {
+ createdAts = append(createdAts, measureEntity.GetCreatedAt().AsTime())
+ }
+ }
+ endTime := time.Time{}
+ if req.TimeRange.GetEnd() != nil {
+ endTime = req.TimeRange.GetEnd().AsTime()
+ }
+ originalBegin := req.TimeRange.GetBegin().AsTime()
+ newBegin, rangeEmpty := clampTimeRangeBegin(originalBegin, endTime, createdAts)
+ if rangeEmpty {
Review Comment:
When clamping makes the time range empty, this returns `emptyMeasureQueryResponse` without `group_statuses`. For schema-aware queries (non-empty `group_mod_revisions`) it would be more consistent to still populate `GroupStatuses` when `gatedStatuses` is non-empty.
```suggestion
if rangeEmpty {
if len(gatedStatuses) > 0 {
return &measurev1.QueryResponse{GroupStatuses: gatedStatuses}, nil
}
```
##########
banyand/liaison/grpc/schema_revision.go:
##########
@@ -0,0 +1,134 @@
+// Licensed to Apache Software Foundation (ASF) under one or more contributor
+// license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright
+// ownership. Apache Software Foundation (ASF) licenses this file to you under
+// the Apache License, Version 2.0 (the "License"); you may
+// not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package grpc
+
+import (
+ "time"
+
+ modelv1 "github.com/apache/skywalking-banyandb/api/proto/banyandb/model/v1"
+)
+
+// clampTimeRangeBegin advances begin to the maximum value in createdAts when begin
+// falls before it (Rule 7: query time-range clamp to schema.CreatedAt).
+// Returns the (possibly clamped) begin time and empty=true when the clamped begin
+// is already after end, which means no data can exist in the resulting range.
+// A zero end is treated as unbounded — empty is never true in that case.
+// createdAts entries that are zero are ignored.
+func clampTimeRangeBegin(begin, end time.Time, createdAts []time.Time) (time.Time, bool) {
+ var maxCreatedAt time.Time
+ for _, ca := range createdAts {
+ if ca.After(maxCreatedAt) {
+ maxCreatedAt = ca
+ }
+ }
+ if maxCreatedAt.IsZero() {
+ return begin, false
+ }
+ if begin.Before(maxCreatedAt) {
+ begin = maxCreatedAt
+ }
+ if !end.IsZero() && begin.After(end) {
+ return begin, true
+ }
+ return begin, false
+}
+
+// checkQueryGate evaluates the per-group ModRevision gate for a query request.
+// It returns the collected group statuses and a shortCircuit flag.
+// When shortCircuit is true the caller must not execute the query and should
+// return a response populated only with the returned group_statuses map.
+// When shortCircuit is false and the returned map is non-empty, all gated
+// groups passed (STATUS_SUCCEED) and the caller should attach the map to the
+// successful query response.
+// An empty groupModRevisions map skips the gate entirely (backward compat).
+func checkQueryGate(
+ groups []string,
+ name string,
+ groupModRevisions map[string]int64,
+ getLocatorRevision func(name, group string) (int64, bool),
+ maxWait time.Duration,
+) (map[string]modelv1.Status, bool) {
+ if len(groupModRevisions) == 0 {
+ return nil, false
+ }
+ groupStatuses := make(map[string]modelv1.Status)
+ for _, g := range groups {
+ clientRev, revOK := groupModRevisions[g]
+ if !revOK || clientRev == 0 {
+ continue
+ }
+ cacheRev, found := getLocatorRevision(name, g)
+ if !found {
+ groupStatuses[g] = modelv1.Status_STATUS_NOT_FOUND
+ continue
+ }
+ switch {
+ case clientRev < cacheRev:
+ groupStatuses[g] = modelv1.Status_STATUS_EXPIRED_SCHEMA
+ case clientRev > cacheRev:
+ currentGroup := g
+ reached := awaitRevisionReached(func() int64 {
+ rev, _ := getLocatorRevision(name, currentGroup)
+ return rev
+ }, clientRev, maxWait)
+ if reached {
+ groupStatuses[g] = modelv1.Status_STATUS_SUCCEED
+ } else {
+ groupStatuses[g] = modelv1.Status_STATUS_SCHEMA_NOT_APPLIED
+ }
Review Comment:
`checkQueryGate` can block up to `maxWait` *per gated group* (it calls `awaitRevisionReached` sequentially inside the loop). For multi-group queries this can exceed the intended wait budget by N× and lead to unexpectedly long request latencies. Consider enforcing a single overall deadline (e.g., compute a deadline once and pass remaining time to each wait, or poll all pending groups until either all catch up or the single maxWait expires).
##########
banyand/liaison/grpc/trace.go:
##########
@@ -475,6 +521,17 @@ func (s *traceService) Query(ctx context.Context, req *tracev1.QueryRequest) (re
}
}()
}
+ gatedStatuses, shortCircuit := checkQueryGate(req.Groups, req.Name, req.GroupModRevisions,
+ func(name, group string) (int64, bool) {
+ loc, ok := s.entityRepo.getLocator(identity{name: name, group: group})
+ return loc.ModRevision, ok
+ }, s.maxWaitDuration)
+ if shortCircuit {
+ return &tracev1.QueryResponse{GroupStatuses: gatedStatuses}, nil
+ }
+ if rangeEmpty := s.applyClamp(req); rangeEmpty {
Review Comment:
If `applyClamp` determines the clamped range is empty, the handler returns `emptyTraceQueryResponse` without `group_statuses`. For requests that opted into gating (`group_mod_revisions` non-empty), consider returning an empty response that still includes `GroupStatuses: gatedStatuses` when available, to keep the response surface consistent.
```suggestion
if rangeEmpty := s.applyClamp(req); rangeEmpty {
if len(gatedStatuses) > 0 {
return &tracev1.QueryResponse{GroupStatuses: gatedStatuses}, nil
}
```
##########
banyand/metadata/schema/schemaserver/service.go:
##########
@@ -169,6 +172,10 @@ func (s *server) Validate() error {
if s.port == 0 {
s.port = 17920
}
+ // Short retention values (<1h) are unusual in production but required by integration tests
+ // that verify the physical-removal path of tombstone GC. Accept the value as given so
+ // operators (and the test harness) retain control; silent clamping previously hid the
+ // §4.7.2 test case.
Review Comment:
This change removes any lower-bound validation for `--schema-server-tombstone-retention` (comment says values <1h are accepted). However the PR description/CHANGES indicate there is a 1h floor. Either enforce the documented floor here (and adjust tests to override in a different way) or update the docs/release notes to match the new behavior so operators aren’t surprised by accidentally configuring very small retentions.
--
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: notifications-unsubscribe@skywalking.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org