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

cli: added --locality #8676

Merged
merged 1 commit into from
Aug 30, 2016
Merged

cli: added --locality #8676

merged 1 commit into from
Aug 30, 2016

Conversation

d4l3k
Copy link
Contributor

@d4l3k d4l3k commented Aug 18, 2016

Builds on #8627


This change is Reviewable

@RaduBerinde
Copy link
Member

:lgtm: (looked at the third commit only)


Review status: 0 of 38 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


roachpb/metadata.go, line 233 [r3] (raw file):

// Type returns the underlying type in string form. This is part of pflag's
// value interface.
func (l Locality) Type() string {

_ Locality


Comments from Reviewable

@d4l3k
Copy link
Contributor Author

d4l3k commented Aug 22, 2016

Review status: 0 of 38 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


roachpb/metadata.go, line 233 [r3] (raw file):

Previously, RaduBerinde wrote…

_ Locality

golint
roachpb/metadata.go:233:1: receiver name should not be an underscore

Comments from Reviewable

@RaduBerinde
Copy link
Member

Review status: 0 of 38 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


roachpb/metadata.go, line 233 [r3] (raw file):

Previously, d4l3k (Tristan Rice) wrote…

golint

roachpb/metadata.go:233:1: receiver name should not be an underscore
Ah, my bad

Comments from Reviewable

@tamird
Copy link
Contributor

tamird commented Aug 22, 2016

Review status: 0 of 38 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


roachpb/metadata.go, line 233 [r3] (raw file):

Previously, RaduBerinde wrote…

Ah, my bad

It can be `func (Locality) Type() String`, though.

Comments from Reviewable

@d4l3k
Copy link
Contributor Author

d4l3k commented Aug 22, 2016

Review status: 0 of 38 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


roachpb/metadata.go, line 233 [r3] (raw file):

Previously, tamird (Tamir Duberstein) wrote…

It can be func (Locality) Type() String, though.

Didn't realize that. Done.

Comments from Reviewable

@d4l3k
Copy link
Contributor Author

d4l3k commented Aug 23, 2016

@RaduBerinde @bdarnell Thoughts on whether I should wait to merge this until the allocator actually takes locality into account?

@d4l3k d4l3k force-pushed the locality-field branch 2 times, most recently from e33b181 to ac5effa Compare August 25, 2016 16:50
@bdarnell
Copy link
Contributor

:lgtm:

It's fine to merge this before it is actually used.


Review status: 0 of 40 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


cli/cliflags/flags.go, line 56 [r6] (raw file):

      Name: "locality",
      Description: `
An ordered, comma-separated list of key-value pairs that describe the topography

This help text should say that this is not yet fully implemented.


Comments from Reviewable

@d4l3k d4l3k force-pushed the locality-field branch 2 times, most recently from addd374 to 3439b5d Compare August 29, 2016 17:13
@d4l3k
Copy link
Contributor Author

d4l3k commented Aug 29, 2016

Review status: 0 of 40 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


cli/cliflags/flags.go, line 56 [r6] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

This help text should say that this is not yet fully implemented.

Done.

Comments from Reviewable

@knz
Copy link
Contributor

knz commented Aug 30, 2016

Seems ok to merge right?

@d4l3k
Copy link
Contributor Author

d4l3k commented Aug 30, 2016

It's being blocked on @BramGruneir's comments on #8786.

@BramGruneir
Copy link
Member

Which comments are you waiting on? I just did another pass of it.

@d4l3k
Copy link
Contributor Author

d4l3k commented Aug 30, 2016

Bram Gruneir
Please add a number of other examples!

Tristan Rice
This info will be folded into the https://www.cockroachlabs.com/docs/configure-replication-zones.html docs. As far as I can tell, none of the other cliflags have redundant examples.

Bram Gruneir
store has a large number of examples, but we should ask @jseldess what he thinks.

Not blocked, but also have yet to respond to

Could you explain what is or what is not implemented? Or link to an issue number or something?

@BramGruneir
Copy link
Member

Ah, I say just merge it for now. We can always add more examples.

@d4l3k
Copy link
Contributor Author

d4l3k commented Aug 30, 2016

Just added some extra examples and added a link to the tracking issue.

@BramGruneir
Copy link
Member

That part of this change LGTM. Manitoba FTW.


Review status: 0 of 18 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


Comments from Reviewable

@d4l3k d4l3k merged commit 20f9815 into cockroachdb:develop Aug 30, 2016
@d4l3k d4l3k deleted the locality-field branch August 30, 2016 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants