-
Notifications
You must be signed in to change notification settings - Fork 2
Fix ScanAt to only query shards with intersecting key ranges #305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Added GetIntersectingRoutes method to Engine to find routes whose key ranges intersect with the scan range [start, end). Modified ScanAt in ShardStore to only query groups whose shard ranges intersect with the requested scan range, avoiding unnecessary work and incorrect results. Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com>
Co-authored-by: bootjp <1306365+bootjp@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a performance and correctness issue in ShardStore.ScanAt by implementing intersection logic to query only shards whose key ranges overlap with the scan range, rather than querying all shards.
Changes:
- Added
GetIntersectingRoutesmethod todistribution.Enginethat returns routes with key ranges intersecting the scan range, correctly handling half-open intervals and unbounded ranges - Modified
ShardStore.ScanAtto useGetIntersectingRoutesinstead of iterating over all shard groups - Added comprehensive unit tests covering single-range, multi-range, boundary conditions, and unbounded scans
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| distribution/engine.go | Adds GetIntersectingRoutes method with proper interval intersection logic and thread-safe read locking |
| distribution/engine_test.go | Comprehensive test coverage for GetIntersectingRoutes including boundary conditions and edge cases |
| kv/shard_store.go | Updates ScanAt to query only intersecting shards, improving efficiency and correctness |
| .gitignore | Adds Go build cache directory to ignore list |
Comments suppressed due to low confidence (1)
kv/shard_store.go:84
- The updated ScanAt method now correctly queries only intersecting shards, which is a significant improvement. However, there's no integration test to verify this behavior end-to-end. Consider adding a test in kv/sharded_integration_test.go similar to TestShardedCoordinatorDispatch that:
- Sets up multiple shards with different key ranges
- Populates keys across multiple shards
- Performs scans that span multiple shards and verifies correct results
- Tests boundary conditions (e.g., scan ending at shard boundary)
This would complement the existing unit tests for GetIntersectingRoutes and ensure the integration works correctly.
func (s *ShardStore) ScanAt(ctx context.Context, start []byte, end []byte, limit int, ts uint64) ([]*store.KVPair, error) {
if limit <= 0 {
return []*store.KVPair{}, nil
}
// Get only the routes whose ranges intersect with [start, end)
intersectingRoutes := s.engine.GetIntersectingRoutes(start, end)
var out []*store.KVPair
for _, route := range intersectingRoutes {
g, ok := s.groups[route.GroupID]
if !ok || g == nil || g.Store == nil {
continue
}
kvs, err := g.Store.ScanAt(ctx, start, end, limit, ts)
if err != nil {
return nil, errors.WithStack(err)
}
out = append(out, kvs...)
}
sort.Slice(out, func(i, j int) bool {
return bytes.Compare(out[i].Key, out[j].Key) < 0
})
if len(out) > limit {
out = out[:limit]
}
return out, nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The
ScanAtmethod inShardStorewas querying all shard groups regardless of whether their key ranges intersected with the scan range, causing unnecessary work and potentially incorrect results.Changes
Added
GetIntersectingRoutestodistribution.Engine: Returns routes whose key ranges[rStart, rEnd)intersect with scan range[start, end). Handles bounded/unbounded ranges correctly (nil end indicates unbounded).Modified
ShardStore.ScanAt: Now queries only groups with intersecting ranges instead of iterating over all groups.Test coverage added in
TestEngineGetIntersectingRoutescovering single-range scans, multi-range scans, boundary conditions, and unbounded ranges.✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.