core: region heartbeat with bucket meta#10120
Conversation
Signed-off-by: tongjian <[email protected]>
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
release-note-none
bufferflies
changed the title
Signed-off-by: bufferflies <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10120 +/- ##
==========================================
+ Coverage 78.42% 78.58% +0.15%
==========================================
Files 518 520 +2
Lines 69493 69670 +177
==========================================
+ Hits 54503 54752 +249
+ Misses 11042 10969 -73
- Partials 3948 3949 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
| @@ -246,6 +246,7 @@ | |||
| if hasBuckets { | |||
| if old := origin.GetBuckets(); buckets[i].GetVersion() > old.GetVersion() { | |||
| region.UpdateBuckets(buckets[i], old) | |||
There was a problem hiding this comment.
If we use SetBucketMeta, do we still need UpdateBuckets?
There was a problem hiding this comment.
No need, but currently, we must keep compatibility; we need to update the bucket also. We can remove this update in a higher version.
| // no limit for followers. | ||
| } | ||
| saveKV, _, _, _ := regionGuide(cctx, region, origin) | ||
| overlaps := bc.PutRegion(region) |
There was a problem hiding this comment.
We firstly call putRegion, and then UpdateBuckets, will this order introduce data race?
There was a problem hiding this comment.
No, UpdateBuckets will only update the point, not affect the bucket meta.
| buckets unsafe.Pointer | ||
| // source is used to indicate region's source, such as Storage/Sync/Heartbeat. | ||
| source RegionSource | ||
| // ref is used to indicate the reference count of the region in root-tree and sub-tree. | ||
| ref atomic.Int32 | ||
| // bucketMeta is used to store the bucket meta reported by tikv. | ||
| bucketMeta *metapb.BucketMeta |
There was a problem hiding this comment.
If buckets and bucketMeta are both nil, which one will we use?
There was a problem hiding this comment.
It will return nil.
There was a problem hiding this comment.
What if either buckets or bucketMeta is nil?
Signed-off-by: bufferflies <[email protected]>
ti-chi-bot
bot
added
size/M
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR introduces bucket metadata handling in region data structures to reduce costs and data race risks from separate bucket report streams. The kvproto dependency is updated across all modules, and RegionInfo now extracts, stores, and propagates bucket metadata from region heartbeats to the syncer client. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/syncer/client.go:
- Around line 246-250: The write to region.bucketMeta via SetBucketMeta after
bc.PutRegion(region) is not synchronized and can race with concurrent readers;
either make bucketMeta access atomic like buckets (use
atomic.StorePointer/atomic.LoadPointer or an atomic.Value inside
SetBucketMeta/GetBucketMeta), or protect bucketMeta with the same mutex used for
buckets, or move the SetBucketMeta call to before bc.PutRegion(region) so the
cached reference is initialized consistently; update SetBucketMeta/GetBucketMeta
and any callers (e.g., UpdateBuckets, GetBuckets, bc.PutRegion) to use the
chosen synchronization mechanism to eliminate the race.
🧹 Nitpick comments (1)
pkg/core/region_test.go (1)
1488-1489: Incomplete comment.The comment on line 1488 appears to be truncated. Consider completing it for clarity.
📝 Suggested fix
- // Inherit false if region + // bucket inheritance is skipped when region has bucketMeta set re.Equal(uint64(1), region.GetBuckets().GetVersion())
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
client/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumtests/integrations/go.sumis excluded by!**/*.sumtools/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
client/go.modgo.modpkg/core/region.gopkg/core/region_test.gopkg/syncer/client.gotests/integrations/go.modtools/go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/core/region.go (1)
pkg/response/region.go (1)
RegionInfo(113-136)
🔇 Additional comments (12)
tests/integrations/go.mod (1)
17-17: Consistent kvproto version with client/go.mod.The kvproto version is properly aligned with the version used in client/go.mod, ensuring test infrastructure uses the same protobuf definitions.
go.mod (1)
38-38: Consistent and coordinated kvproto version across all modules.The root module kvproto version matches client/go.mod and tests/integrations/go.mod, confirming a coordinated dependency update. The file's own documentation (lines 5–8) indicates this follows the project's established pattern for kvproto coordination. No other dependencies were inadvertently modified.
client/go.mod (1)
13-13: Verify kvproto dependency update for proto API compatibility.The kvproto dependency is updated from a December 2, 2025 commit to a January 6, 2026 commit. Confirm that this version change does not introduce breaking changes to the
RegionHeartbeatRequest,RegionHeartbeatResponse, andBucketStatsproto messages that the client library depends on.pkg/core/region_test.go (1)
1460-1490: Test coverage for bucket meta inheritance looks good.The test properly validates:
- Initial state with nil buckets
- Bucket updates via
UpdateBuckets- Bucket inheritance behavior with and without
bucketMetaThe test confirms that when
bucketMetais set on a region, theInheritmethod correctly skips bucket inheritance from the origin, which aligns with the PR objective of using heartbeat-based bucket metadata.pkg/core/region.go (7)
629-635: Consider thread-safety forSetBucketMeta.Unlike
UpdateBucketswhich usesatomic.CompareAndSwapPointer,SetBucketMetadirectly assigns tor.bucketMeta. If this method is called concurrently withGetBuckets()reads on the same region, the Go memory model doesn't guarantee visibility without synchronization.While pointer assignment is atomic at the CPU level for aligned pointers (no torn reads), the lack of synchronization means readers might not see the update immediately or could observe a partially initialized object.
If concurrent access is possible, consider using
atomic.Pointerorunsafe.Pointerwith atomic operations similar to thebucketsfield:♻️ Optional: Atomic access pattern
// Field change in RegionInfo struct: // bucketMeta unsafe.Pointer // *metapb.BucketMeta func (r *RegionInfo) SetBucketMeta(buckets *metapb.Buckets) { meta := &metapb.BucketMeta{ Version: buckets.GetVersion(), Keys: buckets.GetKeys(), } atomic.StorePointer(&r.bucketMeta, unsafe.Pointer(meta)) } // In GetBuckets: // meta := (*metapb.BucketMeta)(atomic.LoadPointer(&r.bucketMeta))
652-666: GetBuckets implementation with bucketMeta fallback looks correct.The logic correctly prioritizes
bucketMeta(from heartbeat) over the legacybucketspointer (from bucket report stream), which aligns with the PR objective. The atomic load for the fallbackbucketsfield is preserved.Note: A new
metapb.Bucketsobject is created on each call whenbucketMetais set. IfGetBuckets()is called frequently, consider caching the result. However, this is likely acceptable given typical usage patterns.
301-304: Inherit logic correctly guards bucket inheritance with bucketMeta check.The updated condition ensures that bucket inheritance from the origin region is skipped when the current region already has
bucketMetapopulated from the heartbeat. This properly prioritizes heartbeat-based bucket metadata over inherited values.
221-221: Interface extension for bucket metadata.The
GetBucketMeta()method is added to support retrieving bucket metadata from region heartbeat requests, enabling the new bucket metadata flow.
248-251: RegionFromHeartbeat correctly populates bucketMeta.The new
bucketMetafield is properly initialized from the heartbeat request during region construction.
94-96: NewbucketMetafield properly documented.The field is correctly placed and commented to explain it stores bucket metadata reported by TiKV via heartbeats.
337-337: Clone correctly copies bucketMeta.Shallow copy of the pointer is appropriate since
bucketMetais treated as immutable once set.tools/go.mod (1)
26-26: Kvproto version is valid and includes the requiredBucketMetatype.The dependency update to
v0.0.0-20260106110113-438649d89ee7is confirmed to contain theBucketMetamessage inpkg/metapb, which provides theversionandkeysfields needed for the bucket metadata feature in region heartbeats.
|
/retest |
ti-chi-bot
bot
added
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lhy1024, rleungx The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test pull-unit-test-next-gen-1 |
3 participants
What problem does this PR solve?
Issue Number: Close #10117
What is changed and how does it work?
Check List
Tests
Code changes
Side effects
Related changes
pingcap/docs/pingcap/docs-cn:pingcap/tiup:Release note
Summary by CodeRabbit
New Features
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.