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/start: write the URL file as soon as the server is ready #39300

Merged
merged 1 commit into from
Aug 5, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 3, 2019

Fixes #38758.

Prior to this patch, cockroach start would wait until the cluster
was initialized to write the SQL URL to the file indicated by
--listening-url-file. This contrasts with the PID file which is
written as soon as the server starts listening.

This prior behavior was motivated by the original design of the
startup routine, where the SQL server was not ready/started until the
cluster was ready.

Since then, #21682 was merged which accepts, but blocks, SQL clients
until the cluster is initialized. Since the SQL clients can now
connect as soon as the server starts listening, there is no reason
any more to wait longer.

This patch thus simplifies the code to emit the URL to the file at the
same time as the PID. This also makes it possible to use a SQL
client/command reliably as soon as the process detaches from the
terminal with --background.

Note: if you want to test the new behavior manually in an interactive
terminal, beware that the cockroach CLI client commands separately
have a connection timeout, which defaults to 5 seconds. This can be
overridden with the env var COCKROACH_CONNECT_TIMEOUT.

Release note (cli change): cockroach start now writes the client URL
to the file specified via --listen-url-file as soon as the server
is ready to accept connections. This also happens before the server
detaches from the terminal when --background is specified.

@knz knz requested review from bdarnell and tbg August 3, 2019 21:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20190803-url branch 4 times, most recently from 0488d8a to f8cfd88 Compare August 4, 2019 12:07
Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

:lgtm: bedankt!

Reviewed 6 of 6 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @bdarnell and @knz)


pkg/cli/start.go, line 564 at r1 (raw file):

		// If the invoker has requested an URL update, do it now that
		// the server is ready to accept SQL connections.

Can you point out why the server is ready to accept SQL connections, so that we're not scared to touch this in future rearrangements in this code?


pkg/cli/interactive_tests/common.tcl, line 92 at r1 (raw file):

    report "BEGIN START SERVER"
    system "$argv start-single-node --insecure --pid-file=server_pid --background -s=path=logs/db >>logs/expect-cmd.log 2>&1;
            $argv sql --insecure -e 'select 1'"

This has the COCKROACH_CONNECT_TIMEOUT default of 5s active, just pointing it out. Maybe you want to bump it preemptively because Docker.... Ditto elsewhere.

Prior to this patch, `cockroach start` would wait until the cluster
was initialized to write the SQL URL to the file indicated by
`--listening-url-file`. This contrasts with the PID file which is
written as soon as the server starts listening.

This prior behavior was motivated by the original design of the
startup routine, where the SQL server was not ready/started until the
cluster was ready.

Since then, cockroachdb#21682 was merged which accepts, but blocks, SQL clients
until the cluster is initialized. Since the SQL clients can now
connect as soon as the server starts listening, there is no reason
any more to wait longer.

This patch thus simplifies the code to emit the URL to the file at the
same time as the PID. This also makes it possible to use a SQL
client/command reliably as soon as the process detaches from the
terminal with `--background`.

Note: if you want to test the new behavior manually in an interactive
terminal, beware that the `cockroach` CLI client commands separately
have a connection timeout, which defaults to 5 seconds. This can be
overridden with the env var COCKROACH_CONNECT_TIMEOUT.

Release note (cli change): `cockroach start` now writes the client URL
to the file specified via `--listen-url-file` as soon as the server
is ready to accept connections. This also happens before the server
detaches from the terminal when `--background` is specified.
Copy link
Contributor Author

@knz knz 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! 1 of 0 LGTMs obtained (waiting on @bdarnell and @tbg)


pkg/cli/start.go, line 564 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Can you point out why the server is ready to accept SQL connections, so that we're not scared to touch this in future rearrangements in this code?

Done.


pkg/cli/interactive_tests/common.tcl, line 92 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

This has the COCKROACH_CONNECT_TIMEOUT default of 5s active, just pointing it out. Maybe you want to bump it preemptively because Docker.... Ditto elsewhere.

Good call. Done.

@knz
Copy link
Contributor Author

knz commented Aug 5, 2019

TFYR!

bors r+

craig bot pushed a commit that referenced this pull request Aug 5, 2019
39289: build: print node version error to stderr r=nvanbenschoten a=nvanbenschoten

Without this, the error was being appended to `pkg/ui/src/js/protos.d.ts`,
which was promptly being deleted, making the failure completely silent.

Release note: None

39300: cli/start: write the URL file as soon as the server is ready r=knz a=knz

Fixes #38758.

Prior to this patch, `cockroach start` would wait until the cluster
was initialized to write the SQL URL to the file indicated by
`--listening-url-file`. This contrasts with the PID file which is
written as soon as the server starts listening.

This prior behavior was motivated by the original design of the
startup routine, where the SQL server was not ready/started until the
cluster was ready.

Since then, #21682 was merged which accepts, but blocks, SQL clients
until the cluster is initialized. Since the SQL clients can now
connect as soon as the server starts listening, there is no reason
any more to wait longer.

This patch thus simplifies the code to emit the URL to the file at the
same time as the PID. This also makes it possible to use a SQL
client/command reliably as soon as the process detaches from the
terminal with `--background`.

Note: if you want to test the new behavior manually in an interactive
terminal, beware that the `cockroach` CLI client commands separately
have a connection timeout, which defaults to 5 seconds. This can be
overridden with the env var COCKROACH_CONNECT_TIMEOUT.

Release note (cli change): `cockroach start` now writes the client URL
to the file specified via `--listen-url-file` as soon as the server
is ready to accept connections. This also happens before the server
detaches from the terminal when `--background` is specified.

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>
@craig
Copy link
Contributor

craig bot commented Aug 5, 2019

Build succeeded

@craig craig bot merged commit b8cc29c into cockroachdb:master Aug 5, 2019
@akshayjshah
Copy link
Contributor

Thanks @knz!

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.

Listening URL file is written after process is backgrounded
4 participants