kvserver: fix flaky work queue tests #113259
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is |
srosenberg
requested review from
a team,
herkolategan and
renatolabs
and removed request for
a team
erikgrinaker
approved these changes
Oct 30, 2023
herkolategan
approved these changes
Nov 13, 2023
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @renatolabs and @srosenberg)
pkg/kv/kvserver/queue_test.go line 670 at r1 (raw file):
// Now check that a queue which doesn't require the system config can // successfully add and process a replica. bqNoSysCfg := makeTestBaseQueue("test2", testQueue, tc.store, queueConfig{
super nit: is there a reason for the name change here, just out of curiosity?
herkolategan
approved these changes
Nov 13, 2023
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/queue_test.go line 670 at r1 (raw file):
Previously, herkolategan (Herko Lategan) wrote…
super nit: is there a reason for the name change here, just out of curiosity?
I'm guessing it is to avoid a possible conflict with bqNeedsSysCfg := makeTestBaseQueue("test", testQueue ... above?
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
srosenberg
force-pushed
the
sr/kvserver_flaky_cluster_setting
branch
from
c5020c4
to
20b6972
Compare
srosenberg
added
backport-23.1.x
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
BoolSettingwhich aliases slot 0 of the global cluster settingregistry. Thus, its value corresponds to whichever cluster setting is registered first duringinit(). Before the change in [2], the first registered cluster setting isbulkio.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 becausebaseQueue.mu.disabledis now true instead of previously, false.The change in this PR fixes the bug by registering a fresh dummy
BoolSetting. We also renamedisabledConfigto match the polarity used in conjunction withbq.SetDisabled.[1] #104170
[2] #113113
Epic: none
Release note: None