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

sql, storage: tolerate existing splits in AdminSplit and SPLIT AT #14273

Merged
merged 2 commits into from
Mar 20, 2017

Conversation

RaduBerinde
Copy link
Member

@RaduBerinde RaduBerinde commented Mar 20, 2017

This error is not useful; indeed many callers go through hoops to ignore it.
Extending SPLIT AT to handle multiple split points makes this error even more
annoying. Removing this error in favor of a silent no-op (based on discussion
in #14146).

The backup test is changed to read the meta descriptor instead on relying on
the old behavior to verify splits.


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 20, 2017

:lgtm:


Reviewed 8 of 8 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/kv/split_test.go, line 231 at r1 (raw file):

	splitKey := roachpb.Key("aa")
	log.Infof(context.Background(), "starting split at key %q...", splitKey)

yikes, this mixing of contexts. while you extract one while you're here?


pkg/storage/client_split_test.go, line 317 at r1 (raw file):

		if pErr != nil {
			// There are only two expected errors from concurrent splits:
			// conflicting range descriptors if the splits are initiated

huh, how come the conflicting descriptor case is absent?


Comments from Reviewable

@RaduBerinde
Copy link
Member Author

pkg/storage/client_split_test.go, line 317 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

huh, how come the conflicting descriptor case is absent?

Sorry, I'm confused by the question. I just removed the "range already split" error case since that's no longer a thing.


Comments from Reviewable

@RaduBerinde
Copy link
Member Author

pkg/storage/client_split_test.go, line 317 at r1 (raw file):

Previously, RaduBerinde wrote…

Sorry, I'm confused by the question. I just removed the "range already split" error case since that's no longer a thing.

Ah, absent from the list of expected errors? No idea, I'll try to hunt down whoever touched this code.


Comments from Reviewable

@RaduBerinde
Copy link
Member Author

pkg/storage/client_split_test.go, line 317 at r1 (raw file):

Previously, RaduBerinde wrote…

Ah, absent from the list of expected errors? No idea, I'll try to hunt down whoever touched this code.

Looks like it was removed in 4b0578a @vivekmenezes do we need to update this comment?


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 20, 2017

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/kv/split_test.go, line 231 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

yikes, this mixing of contexts. while you extract one while you're here?

that should be "can you ...". not sure how i mangled that sentence so badly.


pkg/storage/client_split_test.go, line 317 at r1 (raw file):

Previously, RaduBerinde wrote…

Looks like it was removed in 4b0578a @vivekmenezes do we need to update this comment?

Yeah, looks like you can update the comment and remove the strings.Join dance. Thanks for tracking it down.


Comments from Reviewable

This error is not useful; indeed many callers go through hoops to ignore it.
Extending SPLIT AT to handle multiple split points makes this error even more
annoying. Removing this error in favor of a silent no-op (based on discussion
in cockroachdb#14146).

The backup test is changed to read the meta descriptor instead on relying on
the old behavior to verify splits.
@RaduBerinde
Copy link
Member Author

TFTR!


Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.


pkg/kv/split_test.go, line 231 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

that should be "can you ...". not sure how i mangled that sentence so badly.

Done. Did it for the entire file (in a separate commit).


pkg/storage/client_split_test.go, line 317 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

Yeah, looks like you can update the comment and remove the strings.Join dance. Thanks for tracking it down.

Done.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Mar 20, 2017

Reviewed 1 of 8 files at r1, 1 of 1 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@maddyblue
Copy link
Contributor

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@RaduBerinde RaduBerinde merged commit 212ebd1 into cockroachdb:master Mar 20, 2017
@RaduBerinde RaduBerinde deleted the already-split-noop branch March 20, 2017 19:03
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this pull request Aug 10, 2017
`TestStoreRangeSplitAtRangeBounds` should have been in conflict with cockroachdb#14273,
but the test had been broken for a while because we were sending
`AdminSplit` requests to the wrong range. This change fixes the test by
asserting that attempting to split a range at its start key is a no-op
which does not actually perform a split.

Additionally, we had places in our code that claimed an `AdminSplit`
request to the end of a range was a no-op and other places that claimed
it was an error. In reality, it should have been a `RangeKeyMismatchError`,
so this change removes the incorrect code/comments and asserts this behavior
more directly in the test.

Note that the change in `replica_command.go` does not alter current behavior.
The change simply removes an impossible condition.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this pull request Aug 10, 2017
`TestStoreRangeSplitAtRangeBounds` should have been in conflict with cockroachdb#14273,
but the test had been broken for a while because we were sending
`AdminSplit` requests to the wrong range. This change fixes the test by
asserting that attempting to split a range at its start key is a no-op
which does not actually perform a split.

Additionally, we had places in our code that claimed an `AdminSplit`
request to the end of a range was a no-op and other places that claimed
it was an error. In reality, it should have been a `RangeKeyMismatchError`,
so this change removes the incorrect code/comments and asserts this behavior
more directly in the test.

Note that the change in `replica_command.go` does not alter current behavior.
The change simply removes an impossible condition.
nvanbenschoten added a commit to nvanbenschoten/cockroach that referenced this pull request Aug 10, 2017
`TestStoreRangeSplitAtRangeBounds` should have been in conflict with cockroachdb#14273,
but the test had been broken for a while because we were sending
`AdminSplit` requests to the wrong range. This change fixes the test by
asserting that attempting to split a range at its start key is a no-op
which does not actually perform a split.

Additionally, we had places in our code that claimed an `AdminSplit`
request to the end of a range was a no-op and other places that claimed
it was an error. In reality, it should have been a `RangeKeyMismatchError`,
so this change removes the incorrect code/comments and asserts this behavior
more directly in the test.

Note that the change in `replica_command.go` does not alter current behavior.
The change simply removes an impossible condition.
pav-kv pushed a commit to pav-kv/cockroach that referenced this pull request Mar 5, 2024
tests: fix the logic of testNonleaderElectionTimeoutRandomized in raft_paper_test.go
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.

4 participants