Skip to content

exit codes and no bootstrapping of >1 store#3581

Merged
tbg merged 3 commits intocockroachdb:masterfrom
tbg:exit_code
Jan 4, 2016
Merged

exit codes and no bootstrapping of >1 store#3581
tbg merged 3 commits intocockroachdb:masterfrom
tbg:exit_code

Conversation

@tbg
Copy link
Copy Markdown
Member

@tbg tbg commented Jan 4, 2016

this makes it a little more sane to work with the cli. In particular, a failed init, start, ... now returns an error. Also changed ./cockroach sql so that it returns the error of the last failing command, which makes something like

echo "SELECTTTT 1;" | ./cockroach sql

return a nonzero exit code.

Review on Reviewable

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Jan 4, 2016

Reviewed 1 of 1 files at r1, 11 of 11 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

cli/sql.go, line 93 [r2] (raw file):
what are you trying to achieve with these gymnastics?

cli/start.go, line 234 [r2] (raw file):
i don't understand this - why do some of these panic while others return errors "normally"?

util/stop/stopper.go, line 199 [r1] (raw file):
something unsettling about these goroutines.

Comments from the review on Reviewable.io

@tbg
Copy link
Copy Markdown
Member Author

tbg commented Jan 4, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions.

cli/sql.go, line 93 [r2] (raw file):
returning nil from this function if the input EOF'ed.

cli/start.go, line 234 [r2] (raw file):
in an ideal world all of these would return an error. But that means refactoring a lot of the code by basically adding error handling in thousands of boring places. Seems preferable to just panic when anything goes wrong since we don't care about fixing itand only want a message to print to the user (I tossed the error-handling branch half way through).

util/stop/stopper.go, line 199 [r1] (raw file):
there's something unsettling about a panic. This is a bit of a drive-by; the only thing I really need is that the stopper doesn't just do nothing on a panic (since then my change leaks goroutines due to its "legitimate" use of panic()). I think the original motivation for putting this in was tests not shutting down cleanly. I'm open for suggestions here.

Comments from the review on Reviewable.io

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Jan 4, 2016

Review status: all files reviewed at latest revision, 3 unresolved discussions.

cli/sql.go, line 93 [r2] (raw file):
why return right here?

cli/start.go, line 234 [r2] (raw file):
That sounds OK, but at least a comment explaining what work is being avoided by panicking would be helpful.

util/stop/stopper.go, line 199 [r1] (raw file):
ah, I see. Can you expand the comment explaining why best effort is even needed?

Comments from the review on Reviewable.io

@tbg
Copy link
Copy Markdown
Member Author

tbg commented Jan 4, 2016

Review status: 9 of 12 files reviewed at latest revision, 3 unresolved discussions.

cli/sql.go, line 93 [r2] (raw file):
Good idea, done.

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

util/stop/stopper.go, line 199 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Jan 4, 2016

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

cli/sql.go, line 82 [r3] (raw file):
neither l nor err need to outlive this loop (assuming you remove the unreachable code down below); i'd move them inside

cli/sql.go, line 124 [r3] (raw file):
i'm pretty sure this is unreachable now

cli/sql.go, line 126 [r3] (raw file):
holding onto execErr like this doesn't seem to provide any value - i don't think exiting non-zero when the user has typed erroneous SQL followed by EOF is correct.

Comments from the review on Reviewable.io

tbg added 3 commits January 4, 2016 13:41
@tbg
attempt best-effort stop on panic
6da9f8e 
this is going to be necessary with the next commits
which will use panic() for the cli commands.
@tbg
fix typo
1e50108
@tbg
exit codes and no bootstrapping of >1 store
d8b7ded
@tbg
Copy link
Copy Markdown
Member Author

tbg commented Jan 4, 2016

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

cli/sql.go, line 126 [r3] (raw file):
looked at psql which always exits zero. I'm just going to back out this part of the change.

Comments from the review on Reviewable.io

@tamird
Copy link
Copy Markdown
Contributor

tamird commented Jan 4, 2016

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

Comments from the review on Reviewable.io

tbg added a commit that referenced this pull request Jan 4, 2016
@tbg
Merge pull request #3581 from tschottdorf/exit_code
c934476 
exit codes and no bootstrapping of >1 store
@tbg tbg merged commit c934476 into cockroachdb:master Jan 4, 2016
@tbg tbg deleted the exit_code branch January 4, 2016 20:15
@tbg tbg mentioned this pull request Jan 5, 2016
Merged
@mberhault mberhault mentioned this pull request May 18, 2016
Closed
andreimatei added a commit to andreimatei/cockroach that referenced this pull request Nov 28, 2022
@andreimatei
stop: don't do best effort shutdown on panics
61a8885 
Before this patch, Stopper.Stop() would do a "best-effort" shutdown when
called while a panic is unwinding: it would signal quiescence, and then
run all closer async while swallowing their panics, then wait a bit,
then re-panic. If the stopper is to do any shutdown on panics, this
shutdown has to be somewhat involved, since some closers are known to
panic easily when closed in an unclean state (e.g. memory accounts with
bytes taken out of them panic), and tasks can fail to respond to
quiescence, etc. This is all too complicated, and pretty unsound, for
little, if any, gain, so this patch gets rid of it all in favor of not
doing any shutdown and re-panicking immediately.

This best-effort shutdown was added in cockroachdb#3581, for the purported benefit
of CLI tests that wanted to trigger panic code-paths. The respective
code is long gone - as far as I can dig up, it had to do with ancient,
per-SQL cli kv-client commands.
Over time, this panic recovery became load-bearing in cockroachdb#45983, when
leaktest was modified to not crash the package binary on panics, and
instead continue with further tests. If we are to continue with other
tests after a panicking one, we'd better try to cleanup the panicking
TestServer as best we can. The leaktest panic behavior is reverted in
the prior commit.

Release note: None
andreimatei added a commit to andreimatei/cockroach that referenced this pull request Dec 2, 2022
@andreimatei
stop: don't do best effort shutdown on panics
94a25f8 
Before this patch, Stopper.Stop() would do a "best-effort" shutdown when
called while a panic is unwinding: it would signal quiescence, and then
run all closer async while swallowing their panics, then wait a bit,
then re-panic. If the stopper is to do any shutdown on panics, this
shutdown has to be somewhat involved, since some closers are known to
panic easily when closed in an unclean state (e.g. memory accounts with
bytes taken out of them panic), and tasks can fail to respond to
quiescence, etc. This is all too complicated, and pretty unsound, for
little, if any, gain, so this patch gets rid of it all in favor of not
doing any shutdown and re-panicking immediately.

This best-effort shutdown was added in cockroachdb#3581, for the purported benefit
of CLI tests that wanted to trigger panic code-paths. The respective
code is long gone - as far as I can dig up, it had to do with ancient,
per-SQL cli kv-client commands.
Over time, this panic recovery became load-bearing in cockroachdb#45983, when
leaktest was modified to not crash the package binary on panics, and
instead continue with further tests. If we are to continue with other
tests after a panicking one, we'd better try to cleanup the panicking
TestServer as best we can. The leaktest panic behavior is reverted in
the prior commit.

Release note: None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@tbg @tamird