kvserver: add cluster setting to disable queues#104170
kvserver: add cluster setting to disable queues#104170craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
andrewbaptist
left a comment
There was a problem hiding this comment.
Update the comment in queue.go related to the disabled field. It currently says:
// Some tests in this package disable queues.
But now it is also configurable through these new settings.
Also update the MaybeRemove method in queue.go to check the disabled flag.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @aadityasondhi)
pkg/kv/kvserver/consistency_queue.go line 109 at r1 (raw file):
kvserverbase.ConsistencyQueueEnabled.SetOnChange(&store.cfg.Settings.SV, func(ctx context.Context) { q.SetDisabled(!kvserverbase.ConsistencyQueueEnabled.Get(&store.cfg.Settings.SV)) })
nit: I would do this a little differently since this is the exact same code on every queue. Add a parameter - something like disabledConfig *settings.BoolSetting to queueConfig and then pass in the parameter itself. This will avoid missing it on new queue creations, and it also prevents it from creating a queue and then having to disable it right afterwards.
pkg/kv/kvserver/split_queue.go line 183 at r1 (raw file):
} func (sq *splitQueue) enabled() bool {
Remove this redundant setting.
f9c9b6b to
5e6fb85
Compare
aadityasondhi
left a comment
There was a problem hiding this comment.
TFTR!
I ended up not adding a disabled check to MaybeRemove based on this comment:
Reviewable status:
pkg/kv/kvserver/consistency_queue.go line 109 at r1 (raw file):
Previously, andrewbaptist (Andrew Baptist) wrote…
nit: I would do this a little differently since this is the exact same code on every queue. Add a parameter - something like
disabledConfig *settings.BoolSettingtoqueueConfigand then pass in the parameter itself. This will avoid missing it on new queue creations, and it also prevents it from creating a queue and then having to disable it right afterwards.
Done.
pkg/kv/kvserver/split_queue.go line 183 at r1 (raw file):
Previously, andrewbaptist (Andrew Baptist) wrote…
Remove this redundant setting.
Done.
3bcc2ca to
03f768c
Compare
|
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
6321d47 to
77b87af
Compare
andrewbaptist
left a comment
There was a problem hiding this comment.
Reviewed 13 of 13 files at r2, all commit messages.
Reviewable status:
|
bors r+ |
|
This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried. Additional information: {"message":"Changes must be made through a pull request.","documentation_url":"https://docs.github.com/articles/about-protected-branches"} |
This patch introduces a new set of cluster settings to disable work queues in the `Store` object. It makes use of the underlying `disabled` property of the `BaseQueue`. Fixes: cockroachdb#102031 Release note: None
77b87af to
2b91c38
Compare
|
bors r+ |
|
Build succeeded: |
In [1], new cluster settings have been added, e.g., `kv.replica_gc_queue.enabled`, `kv.raft_log_queue.enabled`, to selectively disable work queues per store, at runtime. The new unit tests use a dummy `BoolSetting` which aliases slot 0 of the global cluster setting `registry`. Thus, its value corresponds to whichever cluster setting is registered first during `init()`. Before the change in [2], the first registered cluster setting is `bulkio.backup.merge_file_buffer_size`, one with a non-zero value in slot 0. After the change, it is `keyvisualizer.enabled`, one with a zero value in slot 0. Subsequently, a number of the unit tests failed because `baseQueue.mu.disabled` is now true instead of previously, false. The change in this PR fixes the bug by registering a fresh dummy `BoolSetting`. We also rename `disabledConfig` to match the polarity used in conjunction with `bq.SetDisabled`. [1] cockroachdb#104170 [2] cockroachdb#113113 Epic: none Release note: None
In [1], new cluster settings have been added, e.g., `kv.replica_gc_queue.enabled`, `kv.raft_log_queue.enabled`, to selectively disable work queues per store, at runtime. The new unit tests use a dummy `BoolSetting` which aliases slot 0 of the global cluster setting `registry`. Thus, its value corresponds to whichever cluster setting is registered first during `init()`. Before the change in [2], the first registered cluster setting is `bulkio.backup.merge_file_buffer_size`, one with a non-zero value in slot 0. After the change, it is `keyvisualizer.enabled`, one with a zero value in slot 0. Subsequently, a number of the unit tests failed because `baseQueue.mu.disabled` is now true instead of previously, false. The change in this PR fixes the bug by registering a fresh dummy `BoolSetting`. We also rename `disabledConfig` to match the polarity used in conjunction with `bq.SetDisabled`. [1] cockroachdb#104170 [2] cockroachdb#113113 Epic: none Release note: None
This patch introduces a new set of cluster settings to disable work queues in the
Storeobject. It makes use of the underlyingdisabledproperty of theBaseQueue.Fixes: #102031
Release note: None