server: enhance comments surrounding draining #76172

cameronnunez · 2022-02-07T17:02:48Z

This patch improves the comments related to the draining process,
fixing incorrect statements and providing more details to make
the process more easily understood.

Release note: None

cockroach-teamcity · 2022-02-07T17:02:56Z

This change is

cameronnunez · 2022-02-07T17:06:58Z

@ZhouXing19 we can be a bit more precise about how we think about the drain sequence. While we have been thinking about it as:

Wait as "not ready"
(WIP) Wait for SQL connections to close
Drain SQL connections
Drain range leases & leadership

Really it's better to think about phase 3 as: Drain the SQL layer. This includes draining SQL connections, draining distributed SQL execution flows, and draining SQL table leases.

cameronnunez · 2022-02-07T17:07:26Z

cc @taroface

taroface · 2022-02-08T15:39:49Z

docs/generated/settings/settings-for-tenants.txt

-server.shutdown.lease_transfer_wait	duration	5s	the amount of time a server waits to transfer range leases before proceeding with the rest of the shutdown process (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
-server.shutdown.query_wait	duration	10s	the server will wait for at least this amount of time for active queries to finish (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
+server.shutdown.drain_wait	duration	0s	the minimum amount of time a server waits in an unready state before proceeding with a drain(note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
+server.shutdown.lease_transfer_wait	duration	5s	the timeout for a single iteration of the range lease transfer phase of draining(note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)


Nit: missing space between "drain"/"draining" and "(note" in the two above settings

Fixed thanks.

taroface · 2022-02-08T15:42:35Z

docs/generated/settings/settings-for-tenants.txt

-server.shutdown.drain_wait	duration	0s	the amount of time a server waits in an unready state before proceeding with the rest of the shutdown process (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
-server.shutdown.lease_transfer_wait	duration	5s	the amount of time a server waits to transfer range leases before proceeding with the rest of the shutdown process (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
-server.shutdown.query_wait	duration	10s	the server will wait for at least this amount of time for active queries to finish (note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)
+server.shutdown.drain_wait	duration	0s	the minimum amount of time a server waits in an unready state before proceeding with a drain(note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)


My understanding is that the unready state is the first step of a drain. So should "before proceeding with a drain" be e.g. "at the beginning of a drain"?

The purpose of the first step is to wait the duration of drainWait before we start draining. So while it is technically a part of the drain sequence, it is not a part of the actual draining phases.

knz

nice, thanks

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cameronnunez and @ZhouXing19)

docs/generated/settings/settings-for-tenants.txt, line 63 at r1 (raw file):

server.oidc_authentication.scopes	string	openid	sets OIDC scopes to include with authentication request (space delimited list of strings, required to start with `openid`)
server.rangelog.ttl	duration	720h0m0s	if nonzero, range log entries older than this duration are deleted every 10m0s. Should not be lowered below 24 hours.
server.shutdown.drain_wait	duration	0s	the minimum amount of time a server waits in an unready state before proceeding with a drain(note that the --drain-wait parameter for cockroach node drain may need adjustment after changing this setting)

why "minimum" here? Is there a way for the initial delay to be longer than the configured value?

pkg/kv/kvserver/store.go, line 1358 at r1 (raw file):

//
// Note: this code represents ONE round of draining. This code is iterated on
// indefinitely until all leases are transferred away.

perhaps you can point the reader to where this iteration can be found.

pkg/server/drain.go, line 174 at r1 (raw file):

// Drain idempotently activates the draining mode.
// Note: this represents ONE round of draining. This code is iterated on
// indefinitely until all range leases have been drained.

ditto

pkg/server/drain.go, line 230 at r1 (raw file):

		return err
	}
	// Finally, mark the node as draining in liveness and drain the

We need to explain the bit about liveness too.

ZhouXing19

! My opinions are just for minor changes.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cameronnunez, @knz, and @taroface)

docs/generated/settings/settings-for-tenants.txt, line 63 at r1 (raw file):

Previously, knz (kena) wrote…

why "minimum" here? Is there a way for the initial delay to be longer than the configured value?

I think it was trying to say that this is a "hard wait" without any early exit.

docs/generated/settings/settings-for-tenants.txt, line 63 at r1 (raw file):

Previously, taroface (Ryan Kuo) wrote…

My understanding is that the unready state is the first step of a drain. So should "before proceeding with a drain" be e.g. "at the beginning of a drain"?

It seems that we try to say this period is not a draining phase, but more of a "warm-up" that waits for the draining to starts.

pkg/server/drain.go, line 248 at r1 (raw file):

	// Set the gRPC mode of the node to "draining" and mark the node as "not ready".
	// Probes to /health?ready=1 will now notice the change in the node's readiness.

nit: "will now notice the node is in a "shutting-down" status, and is unready for new SQL connection."

pkg/server/drain.go, line 261 at r1 (raw file):

	// Drain all SQL connections.
	// The queryWait duration is used to wait on clients to self-disconnect.

Nit: Though this may have been conveyed by the words "wait" and "timeout", maybe we can explicitly add that "Once all existing SQL queries finish before the queryWait timeout, the server early exits this waiting phase and proceeds to the rest of the drain".

knz

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cameronnunez, @taroface, and @ZhouXing19)

docs/generated/settings/settings-for-tenants.txt, line 63 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

I think it was trying to say that this is a "hard wait" without any early exit.

then the good word is "fixed", not "minimum".

cameronnunez

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @taroface, and @ZhouXing19)

docs/generated/settings/settings-for-tenants.txt, line 63 at r1 (raw file):

Previously, knz (kena) wrote…

then the good word is "fixed", not "minimum".

Fixed.

docs/generated/settings/settings-for-tenants.txt, line 64 at r1 (raw file):

Previously, taroface (Ryan Kuo) wrote…

Nit: missing space between "drain"/"draining" and "(note" in the two above settings

Noted fixed.

pkg/kv/kvserver/store.go, line 1358 at r1 (raw file):

Previously, knz (kena) wrote…

perhaps you can point the reader to where this iteration can be found.

Done.

pkg/server/drain.go, line 174 at r1 (raw file):

Previously, knz (kena) wrote…

ditto

Done.

pkg/server/drain.go, line 230 at r1 (raw file):

Previously, knz (kena) wrote…

We need to explain the bit about liveness too.

Done.

pkg/server/drain.go, line 248 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

nit: "will now notice the node is in a "shutting-down" status, and is unready for new SQL connection."

since the mode is modeDraining I think saying "draining" is more appropriate. Also the node can still accept new SQL connections here.

pkg/server/drain.go, line 261 at r1 (raw file):

Previously, ZhouXing19 (Jane Xing) wrote…

Nit: Though this may have been conveyed by the words "wait" and "timeout", maybe we can explicitly add that "Once all existing SQL queries finish before the queryWait timeout, the server early exits this waiting phase and proceeds to the rest of the drain".

More specific about it being a timeout; timeout implies a max duration.

knz

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @cameronnunez, @taroface, and @ZhouXing19)

docs/generated/settings/settings-for-tenants.txt, line 63 at r1 (raw file):

Previously, cameronnunez (Cameron Nunez) wrote…

Fixed.

i don't see it

also there's a space missing before a parenthesis

cameronnunez

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz, @taroface, and @ZhouXing19)

docs/generated/settings/settings-for-tenants.txt, line 63 at r1 (raw file):

Previously, knz (kena) wrote…

i don't see it

also there's a space missing before a parenthesis

ah I was having issues with docs gen. Fixed.

knz

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @taroface and @ZhouXing19)

cameronnunez · 2022-02-10T15:45:55Z

thanks!

bors r=knz

craig · 2022-02-10T16:58:09Z

Build failed:

GitHub CI (Cockroach)

ajwerner · 2022-02-10T17:41:02Z

bors r-

CI failure

cameronnunez · 2022-02-14T17:43:03Z

bors r=knz

craig · 2022-02-14T19:41:15Z

Build failed (retrying...):

GitHub CI (Cockroach)

cameronnunez · 2022-02-14T20:14:45Z

bors r-

craig · 2022-02-14T20:14:49Z

Canceled.

irfansharif · 2022-02-14T20:36:21Z

PS, I think this is tripping up the merge queue due due to unexpected diffs: https://teamcity.cockroachdb.com/viewLog.html?buildId=4372990&problemId=2701

Have you tried dev generate bazel to see if it suggests changes?

cameronnunez · 2022-02-14T21:28:43Z

yeah I've noticed but the regeneration of that file seems odd to me, but I'll add the lines and see what happens with CI

cameronnunez · 2022-02-14T23:17:45Z

looks like I'm running into problems with node_modules because I switched between make build and dev build

This patch improves the comments related to the draining process, fixing incorrect statements and providing more details to make the process more easily understood. Release note: None

cameronnunez · 2022-02-15T20:22:10Z

bors r=knz

craig · 2022-02-15T22:17:22Z

Build succeeded:

GitHub CI (Cockroach)

cameronnunez requested review from knz and ZhouXing19 February 7, 2022 17:02

cameronnunez requested a review from a team as a code owner February 7, 2022 17:02

cameronnunez force-pushed the improve-drain-comments branch 2 times, most recently from 0f46dd8 to f69fee2 Compare February 7, 2022 17:24

taroface reviewed Feb 8, 2022

View reviewed changes

knz reviewed Feb 8, 2022

View reviewed changes

ZhouXing19 reviewed Feb 8, 2022

View reviewed changes

knz reviewed Feb 9, 2022

View reviewed changes

cameronnunez force-pushed the improve-drain-comments branch from f69fee2 to 2d430bd Compare February 9, 2022 16:55

cameronnunez requested a review from a team as a code owner February 9, 2022 16:55

cameronnunez commented Feb 9, 2022

View reviewed changes

cameronnunez requested a review from knz February 9, 2022 16:59

knz reviewed Feb 9, 2022

View reviewed changes

cameronnunez force-pushed the improve-drain-comments branch from 2d430bd to 9c671b5 Compare February 9, 2022 22:46

cameronnunez commented Feb 9, 2022

View reviewed changes

cameronnunez requested a review from knz February 9, 2022 22:47

knz approved these changes Feb 10, 2022

View reviewed changes

cameronnunez force-pushed the improve-drain-comments branch from 9c671b5 to d95bef7 Compare February 10, 2022 15:44

cameronnunez force-pushed the improve-drain-comments branch 3 times, most recently from f5e98d7 to 147552f Compare February 14, 2022 15:44

cameronnunez force-pushed the improve-drain-comments branch from 147552f to 74e60bd Compare February 14, 2022 20:24

cameronnunez force-pushed the improve-drain-comments branch 2 times, most recently from f7bae40 to 5f33e8f Compare February 15, 2022 14:21

server: enhance comments surrounding draining

b7dfd62

This patch improves the comments related to the draining process, fixing incorrect statements and providing more details to make the process more easily understood. Release note: None

cameronnunez force-pushed the improve-drain-comments branch from 5f33e8f to b7dfd62 Compare February 15, 2022 15:37

craig bot merged commit 1452123 into cockroachdb:master Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: enhance comments surrounding draining #76172

server: enhance comments surrounding draining #76172

cameronnunez commented Feb 7, 2022

cockroach-teamcity commented Feb 7, 2022

cameronnunez commented Feb 7, 2022 •

edited

Loading

cameronnunez commented Feb 7, 2022

taroface Feb 8, 2022 •

edited

Loading

cameronnunez Feb 9, 2022

taroface Feb 8, 2022

cameronnunez Feb 8, 2022 •

edited

Loading

knz left a comment

ZhouXing19 left a comment

knz left a comment

cameronnunez left a comment

knz left a comment

cameronnunez left a comment

knz left a comment

cameronnunez commented Feb 10, 2022

craig bot commented Feb 10, 2022

ajwerner commented Feb 10, 2022

cameronnunez commented Feb 14, 2022

craig bot commented Feb 14, 2022

cameronnunez commented Feb 14, 2022

craig bot commented Feb 14, 2022

irfansharif commented Feb 14, 2022

cameronnunez commented Feb 14, 2022

cameronnunez commented Feb 14, 2022

cameronnunez commented Feb 15, 2022

craig bot commented Feb 15, 2022

server: enhance comments surrounding draining #76172

server: enhance comments surrounding draining #76172

Conversation

cameronnunez commented Feb 7, 2022

cockroach-teamcity commented Feb 7, 2022

cameronnunez commented Feb 7, 2022 • edited Loading

cameronnunez commented Feb 7, 2022

taroface Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

cameronnunez Feb 9, 2022

Choose a reason for hiding this comment

taroface Feb 8, 2022

Choose a reason for hiding this comment

cameronnunez Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

ZhouXing19 left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

cameronnunez left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

cameronnunez left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

cameronnunez commented Feb 10, 2022

craig bot commented Feb 10, 2022

ajwerner commented Feb 10, 2022

cameronnunez commented Feb 14, 2022

craig bot commented Feb 14, 2022

cameronnunez commented Feb 14, 2022

craig bot commented Feb 14, 2022

irfansharif commented Feb 14, 2022

cameronnunez commented Feb 14, 2022

cameronnunez commented Feb 14, 2022

cameronnunez commented Feb 15, 2022

craig bot commented Feb 15, 2022

cameronnunez commented Feb 7, 2022 •

edited

Loading

taroface Feb 8, 2022 •

edited

Loading

cameronnunez Feb 8, 2022 •

edited

Loading