Skip to content

AdminMerge: make check for collocated replicas transactional#2431

Merged
bdarnell merged 2 commits intocockroachdb:masterfrom
bdarnell:test-merge-non-collocated
Sep 9, 2015
Merged

AdminMerge: make check for collocated replicas transactional#2431
bdarnell merged 2 commits intocockroachdb:masterfrom
bdarnell:test-merge-non-collocated

Conversation

@bdarnell
Copy link
Copy Markdown
Contributor

@bdarnell bdarnell commented Sep 9, 2015

This lets us fix TestStoreRangeMergeNonConsecutive (which was misnamed)
without relying on delicate operations like Store.RemoveReplica.

Fixes #2363.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why'd this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AdminMerge takes any key in the left-hand side of the range, but the split key is in the right-hand side. This call was trying to merge ["g", "\xff\xff"] with its non-existent following range instead of merging the two ranges mentioned in the comment. Now that AdminMerge returns an error in this case instead of silently doing nothing, it detected this bug.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks for the detailed explanation.

@bdarnell bdarnell mentioned this pull request Sep 9, 2015
9 tasks
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment for this test needs an update.

This lets us fix TestStoreRangeMergeNonConsecutive (which was misnamed)
without relying on delicate operations like Store.RemoveReplica.

Fixes cockroachdb#2363.
@bdarnell bdarnell force-pushed the test-merge-non-collocated branch from 58636b2 to 68a26f4 Compare September 9, 2015 21:05
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

desc1Key, can you rename this to match updatedDescKey or something like that to match updatedDesc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to "left" and "right".

@BramGruneir
Copy link
Copy Markdown
Member

LGTM with some minor renaming.

bdarnell added a commit that referenced this pull request Sep 9, 2015
AdminMerge: make check for collocated replicas transactional
@bdarnell bdarnell merged commit ef07056 into cockroachdb:master Sep 9, 2015
@bdarnell bdarnell deleted the test-merge-non-collocated branch September 9, 2015 22:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test timeout on TestStoreRangeMergeNonConsecutive

4 participants