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

release-22.2: ccl/multiregionccl: disable transfers in multiregion test #110053

Merged
merged 2 commits into from
Sep 6, 2023

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Sep 5, 2023

Backport 2/2 commits from #109819.

/cc @cockroachdb/release


Previously, TestMultiRegionDataDriven allowed kvserver (replicate queue) lease transfers to occur for lease count rebalancing. Load based rebalancing was disabled in an earlier patch #109050, however we saw in interleaving replication changes -- leading to flaky behavior.

Bump the kv.allocator.min_lease_transfer_interval to 5 minutes, effectively disabling internal lease transfers. Lease transfers which are required for lease preference satisfaction, and manual transfers are still permitted.

Fixes: #109215
Release note: None


TestMultiRegionDataDriven speeds up replication changes by proactively
enqueuing replicas into various queues. This has the benefit of reducing
the time taken after zone config changes, however the downside of added
overhead.

Disable the test under deadlock builds, as the test is slow and
susceptible to (legitimate) timing issues on a deadlock build.

Informs: #109215
Release note: None


Release justification: Test only change.

@blathers-crl
Copy link

blathers-crl bot commented Sep 5, 2023

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Patches should only be created for serious issues or test-only changes.
  • Patches should not break backwards-compatibility.
  • Patches should change as little code as possible.
  • Patches should not change on-disk formats or node communication protocols.
  • Patches should not add new functionality.
  • Patches must not add, edit, or otherwise modify cluster versions; or add version gates.
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters.
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.

Add a brief release justification to the body of your PR to justify this backport.

Some other things to consider:

  • What did we do to ensure that a user that doesn’t know & care about this backport, has no idea that it happened?
  • Will this work in a cluster of mixed patch versions? Did we test that?
  • If a user upgrades a patch version, uses this feature, and then downgrades, what happens?

@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Sep 5, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli marked this pull request as ready for review September 5, 2023 18:52
@kvoli kvoli requested a review from a team as a code owner September 5, 2023 18:52
@kvoli kvoli requested a review from rafiss September 5, 2023 18:52
@kvoli kvoli self-assigned this Sep 5, 2023
@kvoli kvoli marked this pull request as draft September 5, 2023 19:00
@kvoli kvoli removed the request for review from rafiss September 5, 2023 19:00
Previously, `TestMultiRegionDataDriven` allowed kvserver (replicate
queue) lease transfers to occur for lease count rebalancing. Load based
rebalancing was disabled in an earlier patch cockroachdb#109050, however we saw in
interleaving replication changes -- leading to flaky behavior.

Bump the `kv.allocator.min_lease_transfer_interval` to 5 minutes,
effectively disabling internal lease transfers. Lease transfers which
are required for lease preference satisfaction, and manual transfers are
still permitted.

Fixes: cockroachdb#109215
Release note: None
`TestMultiRegionDataDriven` speeds up replication changes by proactively
enqueuing replicas into various queues. This has the benefit of reducing
the time taken after zone config changes, however the downside of added
overhead.

Disable the test under deadlock builds, as the test is slow and
susceptible to (legitimate) timing issues on a deadlock build.

Informs: cockroachdb#109215
Release note: None
@kvoli kvoli force-pushed the backport22.2-109819 branch from a53fe5e to 9a4d6ec Compare September 5, 2023 19:25
@kvoli kvoli marked this pull request as ready for review September 6, 2023 15:17
@kvoli kvoli requested a review from rafiss September 6, 2023 15:17
@kvoli kvoli linked an issue Sep 6, 2023 that may be closed by this pull request
@kvoli
Copy link
Collaborator Author

kvoli commented Sep 6, 2023

TYFTR

@kvoli kvoli merged commit 9869f34 into cockroachdb:release-22.2 Sep 6, 2023
kvoli added a commit to kvoli/cockroach that referenced this pull request Sep 6, 2023
Update the cluster setting statement to correctly surround the value
duration in quotations.`5m` to `'5m'`. This was fixed in backports to
release-23.1 (cockroachdb#110052) and release-22.2 (cockroachdb#110053). However, this was not
caught on master due to `TestMultiRegionDataDriven` being skipped.

Informs: cockroachdb#98020
Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ccl/multiregionccl: TestMultiRegionDataDriven failed
3 participants