storage: add cluster setting to expire sticky bits#37728
storage: add cluster setting to expire sticky bits#37728craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
57ee310 to
e337f57
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Nice
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jeffrey-xiao, and @nvanbenschoten)
pkg/storage/merge_queue.go, line 65 at r1 (raw file):
// a LHS and a RHS, then the merge queue will not consider to merge the RHS // with the range to its left until ManualSplitTTL time has passed. var ManualSplitTTL = settings.RegisterDurationSetting(
RegisterNonNegativeDurationSetting is probably a better choice.
pkg/storage/merge_queue.go, line 67 at r1 (raw file):
var ManualSplitTTL = settings.RegisterDurationSetting( "kv.range_merge.manual_split.ttl", "if nonzero, manual splits newer than this duration will not be considered for automatic merging.",
The linter will complain about that ., just drop it
pkg/storage/merge_queue.go, line 307 at r1 (raw file):
if rhsDesc.StickyBit != nil { manualSplitTTL := ManualSplitTTL.Get(&mq.store.ClusterSettings().SV) manualSplitExpired := manualSplitTTL != 0 && rhsDesc.StickyBit.GoTime().Add(manualSplitTTL).Before(mq.store.Clock().Now().GoTime())
This doesn't use the logical portion of the HLC, no reason to pay for it. How about mq.store.Clock().PhysicalTime()?
e337f57 to
d1be00b
Compare
jeffrey-xiao
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/storage/merge_queue.go, line 65 at r1 (raw file):
Previously, ajwerner wrote…
RegisterNonNegativeDurationSettingis probably a better choice.
Done.
pkg/storage/merge_queue.go, line 67 at r1 (raw file):
Previously, ajwerner wrote…
The linter will complain about that
., just drop it
Done.
pkg/storage/merge_queue.go, line 307 at r1 (raw file):
Previously, ajwerner wrote…
This doesn't use the logical portion of the HLC, no reason to pay for it. How about
mq.store.Clock().PhysicalTime()?
Done.
nvb
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:
pkg/storage/client_merge_test.go, line 3288 at r1 (raw file):
storagebase.MergeQueueEnabled.Override(sv, true) storage.MergeQueueInterval.Override(sv, 0) // process greedily storage.ManualSplitTTL.Override(sv, time.Hour)
pull time.Hour into a constant so you can use it here and below.
pkg/storage/client_merge_test.go, line 3474 at r1 (raw file):
}) t.Run("merge split ttl", func(t *testing.T) {
"sticky bit ttl"?
pkg/storage/merge_queue.go, line 62 at r1 (raw file):
// ManualSplitTTL is the amount of time that the merge queue will not consider // a manually split range for merging if it is nonzero. If a range is split in
And what if it is zero? Are all manual splits considered for merges or are none?
pkg/storage/merge_queue.go, line 67 at r1 (raw file):
var ManualSplitTTL = settings.RegisterDurationSetting( "kv.range_merge.manual_split.ttl", "if nonzero, manual splits newer than this duration will not be considered for automatic merging.",
I think it would be clearer if we inverted this phrasing. Something like "if nonzero, manual splits older than this duration will be considered for automatic Range merging."
adafe2d to
f7e4643
Compare
nvb
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:
Since we represent sticky bits as HLCs, we can provide a setting to have them expire after a certain duration. After this duration, these manually split ranges will be eligible for automatic merging again. This setting is useful for cases when a user might want to split the data as it's being loaded into tables, but after the loading is complete, the user would want the manual splits be merged as determined by the merge queue. Resolves: cockroachdb#37696 Release note (general change): Add kv.range_merge.manual_split_ttl setting
f7e4643 to
07a451c
Compare
ajwerner
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3.
Reviewable status:
|
bors r+ |
37728: storage: add cluster setting to expire sticky bits r=jeffrey-xiao a=jeffrey-xiao Since we represent sticky bits as HLCs, we can provide a setting to have them expire after a certain duration. After this duration, these manually split ranges will be eligible for automatic merging again. This setting is useful for cases when a user might want to split the data as it's being loaded into tables, but after the loading is complete, the user would want the manual splits be merged as determined by the merge queue. Resolves: #37696 Release note (general change): Add kv.range_merge.manual_split_ttl setting Co-authored-by: Jeffrey Xiao <jeffrey.xiao1998@gmail.com>
Build succeeded |
Reverts cockroachdb#37728. A cluster setting is coarse to be useful. There could be instances where multiple people are working in the same cluster and there would be conflicts on when they want their sticky bits to expire. Release note(general change): Remove kv.range_merge.manual_split_ttl setting
Reverts cockroachdb#37728. A cluster setting is coarse to be useful. There could be instances where multiple people are working in the same cluster and there would be conflicts on when they want their sticky bits to expire. Release note(general change): Remove kv.range_merge.manual_split_ttl setting
Since we represent sticky bits as HLCs, we can provide a setting to have
them expire after a certain duration. After this duration, these
manually split ranges will be eligible for automatic merging again.
This setting is useful for cases when a user might want to split the
data as it's being loaded into tables, but after the loading is
complete, the user would want the manual splits be merged as determined
by the merge queue.
Resolves: #37696
Release note (general change): Add kv.range_merge.manual_split_ttl setting