Skip to content
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

storage: add cluster setting to expire sticky bits #37728

Merged
merged 1 commit into from May 24, 2019

Conversation

Projects
None yet
4 participants
@jeffrey-xiao
Copy link
Collaborator

commented May 22, 2019

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

@jeffrey-xiao jeffrey-xiao requested a review from cockroachdb/core-prs as a code owner May 22, 2019

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented May 22, 2019

This change is Reviewable

@jeffrey-xiao jeffrey-xiao requested review from ajwerner and nvanbenschoten May 22, 2019

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:manual-split-ttl branch 2 times, most recently from 57ee310 to e337f57 May 22, 2019

@ajwerner
Copy link
Collaborator

left a comment

Nice

Reviewable status: :shipit: 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()?

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:manual-split-ttl branch from e337f57 to d1be00b May 22, 2019

@jeffrey-xiao
Copy link
Collaborator Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @nvanbenschoten)


pkg/storage/merge_queue.go, line 65 at r1 (raw file):

Previously, ajwerner wrote…

RegisterNonNegativeDurationSetting is 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.

@nvanbenschoten
Copy link
Member

left a comment

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jeffrey-xiao, and @nvanbenschoten)


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."

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:manual-split-ttl branch 4 times, most recently from adafe2d to f7e4643 May 22, 2019

@jeffrey-xiao jeffrey-xiao requested review from nvanbenschoten and ajwerner and removed request for nvanbenschoten May 23, 2019

@nvanbenschoten
Copy link
Member

left a comment

:lgtm:

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

storage: add cluster setting to expire sticky bits
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

@jeffrey-xiao jeffrey-xiao force-pushed the jeffrey-xiao:manual-split-ttl branch from f7e4643 to 07a451c May 24, 2019

@ajwerner
Copy link
Collaborator

left a comment

:lgtm:

Reviewed 3 of 3 files at r3.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)

@jeffrey-xiao

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

bors r+

craig bot pushed a commit that referenced this pull request May 24, 2019

Merge #37728
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>
@craig

This comment has been minimized.

Copy link

commented May 24, 2019

Build succeeded

@craig craig bot merged commit 07a451c into cockroachdb:master May 24, 2019

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

jeffrey-xiao added a commit to jeffrey-xiao/cockroach that referenced this pull request Jun 4, 2019

storage: revert cluster setting to expire sticky bits
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

jeffrey-xiao added a commit to jeffrey-xiao/cockroach that referenced this pull request Jun 4, 2019

storage: revert cluster setting to expire sticky bits
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.