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

storage: restore range information in AdminScatterResponse #17409

Merged
merged 1 commit into from
Aug 8, 2017

Conversation

benesch
Copy link
Contributor

@benesch benesch commented Aug 2, 2017

ALTER TABLE... SCATTER expects to receive a list of ranges that were
scattered. This information was accidentally dropped in the new
scatter implementation (dbd90cf, #16249). This commit restores the old
behavior, and adds a test to boot.

Fixes #17153.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@benesch benesch force-pushed the scatter-resp branch 2 times, most recently from bce1354 to 59abcf4 Compare August 7, 2017 14:44
@benesch benesch requested review from a team August 7, 2017 14:44
@tamird
Copy link
Contributor

tamird commented Aug 7, 2017

:lgtm:


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


pkg/sql/split_at_test.go, line 225 at r1 (raw file):

	i := 0
	for ; rows.Next(); i++ {

don't forget to check rows.Err at the end?


Comments from Reviewable

`ALTER TABLE... SCATTER` expects to receive a list of ranges that were
scattered. This information was accidentally dropped in the new
scatter implementation (dbd90cf, cockroachdb#16249). This commit restores the old
behavior, and adds a test to boot.

Fixes cockroachdb#17153.
@benesch
Copy link
Contributor Author

benesch commented Aug 7, 2017

Review status: 1 of 2 files reviewed at latest revision, 1 unresolved discussion.


pkg/sql/split_at_test.go, line 225 at r1 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

don't forget to check rows.Err at the end?

Good call. Done.


Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Aug 7, 2017

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@benesch benesch merged commit fd1e221 into cockroachdb:master Aug 8, 2017
@benesch benesch deleted the scatter-resp branch August 8, 2017 18:10
benesch added a commit to benesch/cockroach that referenced this pull request Aug 8, 2017
storage: restore range information in AdminScatterResponse
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.

3 participants