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: Add per replica locality information to show ranges. #39813

Merged
merged 1 commit into from
Sep 4, 2019

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Aug 22, 2019

Fixes #39793.

Release note (sql change): Show ranges now displays per replica
locality information.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rohany rohany requested review from solongordon and a team August 22, 2019 01:08
@rohany rohany force-pushed the show_ranges_replica_locality branch from d6ed540 to 47ecc9f Compare August 26, 2019 18:46
@rohany
Copy link
Contributor Author

rohany commented Aug 28, 2019

Bump

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @solongordon)


pkg/sql/crdb_internal.go, line 1855 at r1 (raw file):

  index_name           STRING NOT NULL,
	replicas             INT[] NOT NULL,	
  replica_localities   STRING[] NOT NULL,

Whitespace weirdness


pkg/sql/crdb_internal.go, line 1943 at r1 (raw file):

			}

			var voterReplicaLocalities []string

Nit: This slice seems unnecessary. You can just append to replicaLocalityArr as you iterate through voterReplicas.


pkg/sql/show_ranges_test.go, line 53 at r1 (raw file):

		expected := "{"
		for i, replica := range replicas {
			expected = expected + fmt.Sprintf("\"region=test,dc=dc%d\"", replica)

Nit: You can use += here. (In general strings.Builder is better but unnecessary in a unit test.)

Also, you can use the backtick to avoid escaping double quotes: `"region=test,dc=dc%d"`

@rohany rohany force-pushed the show_ranges_replica_locality branch from 47ecc9f to d182d9f Compare August 29, 2019 16:48
Copy link
Contributor Author

@rohany rohany left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @solongordon)


pkg/sql/crdb_internal.go, line 1855 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Whitespace weirdness

what is the rule for this -- is it always supposed to be spaces?? I get this wrong on every PR i send.


pkg/sql/crdb_internal.go, line 1943 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Nit: This slice seems unnecessary. You can just append to replicaLocalityArr as you iterate through voterReplicas.

Good catch.


pkg/sql/show_ranges_test.go, line 53 at r1 (raw file):

Previously, solongordon (Solon) wrote…

Nit: You can use += here. (In general strings.Builder is better but unnecessary in a unit test.)

Also, you can use the backtick to avoid escaping double quotes: `"region=test,dc=dc%d"`

Done

@rohany
Copy link
Contributor Author

rohany commented Aug 29, 2019

RFAL

Copy link
Contributor

@solongordon solongordon left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rohany and @solongordon)


pkg/sql/crdb_internal.go, line 1855 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

what is the rule for this -- is it always supposed to be spaces?? I get this wrong on every PR i send.

Yeah, I think spaces in strings, tabs for code. I always just match whatever the surrounding lines do. GoLand seems to get it right for me.

@rohany
Copy link
Contributor Author

rohany commented Sep 4, 2019

lol every error with spacing i have looks fine in goland :(

Fixes cockroachdb#39793.

Release note (sql change): Show ranges now displays per replica
locality information.
@rohany rohany force-pushed the show_ranges_replica_locality branch from d182d9f to 32185e6 Compare September 4, 2019 00:49
@rohany
Copy link
Contributor Author

rohany commented Sep 4, 2019

bors r=solongordon

craig bot pushed a commit that referenced this pull request Sep 4, 2019
39813: sql: Add per replica locality information to show ranges. r=solongordon a=rohany

Fixes #39793.

Release note (sql change): Show ranges now displays per replica
locality information.

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
@craig
Copy link
Contributor

craig bot commented Sep 4, 2019

Build succeeded

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.

sql: Make SHOW RANGES return all localities per range
3 participants