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: Print more info to stdout when starting a node #9066

Merged
merged 1 commit into from
Sep 7, 2016

Conversation

a-robinson
Copy link
Contributor

@a-robinson a-robinson commented Sep 2, 2016

As mentioned in #8650 (comment), I could use a little more explanation around the intended behavior when trying to join a cluster when a local store already exists, but otherwise this provides a little useful more info to stdout when starting a node.

Closes #8650

@mberhault @sploiselle


This change is Reviewable

@knz
Copy link
Contributor

knz commented Sep 3, 2016

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


cli/start.go, line 403 [r1] (raw file):

      if nodeID == server.FirstNodeID {
          fmt.Fprintf(tw, "Initialized new cluster\n")
          joinStr = "failed to join"

I'm not sure the message "failed to join" is an appropriate description for this situation.


Comments from Reviewable

if initialBoot {
if nodeID == server.FirstNodeID {
fmt.Fprintf(tw, "Initialized new cluster\n")
joinStr = "failed to join"
Copy link
Contributor

Choose a reason for hiding this comment

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

we can't have a non-empty JoinList and initialize a new cluster. This means that failed to join will never be printed.

@a-robinson
Copy link
Contributor Author

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


cli/start.go, line 403 [r1] (raw file):

Previously, mberhault (marc) wrote…

we can't have a non-empty JoinList and initialize a new cluster. This means that failed to join will never be printed.

Thanks for the explanation, that's good to hear.

cli/start.go, line 403 [r1] (raw file):

Previously, knz (kena) wrote…

I'm not sure the message "failed to join" is an appropriate description for this situation.

Done.

cli/start.go, line 415 [r1] (raw file):

Previously, mberhault (marc) wrote…

the problem with the JoinList is that it sounds like something more than just a flag. Unless we have more details on which address was good, I'd say we need to de-emphasize.

Ack, I've removed for now.

Comments from Reviewable

@a-robinson
Copy link
Contributor Author

a-robinson commented Sep 6, 2016

Throwing on the docs-todo label because this changes the output when running cockroach start to something more like this:

build:     beta-20160829-411-gb86c7bc-dirty @ 2016/09/06 12:51:18 (go1.7)
admin:     http://localhost:8080
sql:       postgresql://root@localhost:26257?sslmode=disable
logs:      cockroach-data/logs
store[0]:  path=cockroach-data
status:     initialized new cluster
clusterID:  {e27f29d8-e2d8-4412-99e7-112dd8711984}
nodeID:     1

Or this:

build:     beta-20160829-411-gb86c7bc-dirty @ 2016/09/06 12:51:18 (go1.7)
admin:     http://localhost:8080
sql:       postgresql://root@localhost:26257?sslmode=disable
logs:      cockroach-data/logs
store[0]:  path=cockroach-data
status:     restarted pre-existing node
clusterID:  {8414f587-d249-4ae3-bc33-53c0a6ac1a61}
nodeID:     1

Or this:

build:     beta-20160829-411-gb86c7bc-dirty @ 2016/09/06 12:51:18 (go1.7)
admin:     http://localhost:8081
sql:       postgresql://root@localhost:26258?sslmode=disable
logs:      cockroach-data2/logs
store[0]:  path=cockroach-data2
status:     initialized new node, joined pre-existing cluster
clusterID:  {e27f29d8-e2d8-4412-99e7-112dd8711984}
nodeID:     2

@knz
Copy link
Contributor

knz commented Sep 6, 2016

Ok LGTM but let me bikeshed this just once more. I think the word "existing" in the messages doesn't carry what's happening well enough, especially in the case "existing node to existing cluster". Also it doesn't follow the pattern of the other lines that start with a prefix:. What about:

status: first node in cluster, initialized new cluster
status: initialized new node in existing cluster
status: node restarted, joined previous cluster

or something to the same effect.


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


cli/start.go, line 406 [r2] (raw file):

      }
  } else {
      fmt.Fprintf(tw, "Restarted existing node in existing cluster\n")

The word "existing" here is confusing.


Comments from Reviewable

@a-robinson
Copy link
Contributor Author

Sure, prefixing them all with "status:" makes for nice consistency with the pattern. I also tried clarifying the messages a bit


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


Comments from Reviewable

Also stop printing the --join addresses, since it's not made at all
clear which of them was joined to.
@knz
Copy link
Contributor

knz commented Sep 6, 2016

:lgtm:


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


Comments from Reviewable

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.

4 participants