cli: modernize the SQL API calls #76434
Conversation
|
This change is |
This brings the cli (sub-)packages kicking and screaming into the post-go1.8 era, by adding the missing context arguments. Release note (bug fix): Some of the `cockroach node` subcommands did not handle `--timeout` properly. This is now fixed.
knz
force-pushed
the
20220211-sql-cancel2
branch
from
f5f4316 to
aa30839
Compare
There was a problem hiding this comment.
nice!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt, @knz, and @otan)
pkg/cli/node.go, line 65 at r1 (raw file):
ctx := context.Background() // TODO(knz): This can use a context deadline instead, now that
might be worth keeping as-is anyway, so that timeouts still work when connected to older CRDB versions. (or mention that in this comment.) ditto in other comments.
pkg/cli/clisqlclient/conn.go, line 166 at r1 (raw file):
// // TODO(knz): CockroachDB servers do not properly fill SQLSTATE // (28P01) for password auth errors, so we have to "make do"
Would it be valuable to extend the change in #69106 so it uses 28P01 for passwords? Initially, I intentionally didn't use 28P01, but seems worthwhile if it improves UX
There was a problem hiding this comment.
Reviewable status:
pkg/cli/node.go, line 65 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
might be worth keeping as-is anyway, so that timeouts still work when connected to older CRDB versions. (or mention that in this comment.) ditto in other comments.
This change is backward-compatible! Previous versions know how to parse string INTERVAL values too.
pkg/cli/clisqlclient/conn.go, line 166 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
Would it be valuable to extend the change in #69106 so it uses 28P01 for passwords? Initially, I intentionally didn't use 28P01, but seems worthwhile if it improves UX
Yes I agree. Different PR though, and then we'll need to wait at least 1 version to use it here.
|
bors r=rafiss |
There was a problem hiding this comment.
Reviewable status:
pkg/cli/node.go, line 65 at r1 (raw file):
Previously, knz (kena) wrote…
This change is backward-compatible! Previous versions know how to parse string INTERVAL values too.
oh sorry, to clarify, i meant "it might be better not to do the task mentioned in the TODO comment"
if the CLI were to only use context deadlines, then timeouts wouldn't work against older CRDB versions.
There was a problem hiding this comment.
Reviewable status:
pkg/cli/node.go, line 65 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
oh sorry, to clarify, i meant "it might be better not to do the task mentioned in the TODO comment"
if the CLI were to only use context deadlines, then timeouts wouldn't work against older CRDB versions.
Correct. But after 22.1 is released, we can safely address the todo.
76427: cli,sql: do not exit interactive shells with Ctrl+C r=rafiss a=knz First commit from #76434. Needed for #76433. This change is a preface to making the interactive shell support Ctrl+C to cancel a currently executing query. When that happens, we need to ensure that Ctrl+C *only* cancels the current query, and not the entire shell. This is because there is no deterministic boundary in time for a user to decide whether the key press will cancel only the query, or stop the shell. If they care about keeping their shell alive, they would never "dare" to press Ctrl+C while a query is executing. Incidentally, this is also what `psql` does, for pretty much the same reason. Note that the new behavior is restricted to *interactive* shells, when stdin is a terminal. The behavior for non-interactive shells remains unchanged, where Ctrl+C will terminate everything. This is also what other SQL shells do. Release note (cli change): `cockroach sql` (and `demo`) now continue to accept user input when Ctrl+C is pressed at the interactive prompt and the current input line is empty. Previously, it would terminate the shell. To terminate the shell, the client-side command `\q` is still supported. The user can also terminate the input altogether via EOF (Ctrl+D). The behavior in non-interactive use remains unchanged. Co-authored-by: Raphael 'kena' Poss <[email protected]>
|
bors seems confused about this one because this commit was merged in #76427. bors r- |
|
Canceled. |
Needed for #76433.
This brings the cli (sub-)packages kicking and screaming into the
post-go1.8 era, by adding the missing context arguments.
Also,
Fixes #76432.
Release note (bug fix): Some of the
cockroach nodesubcommands didnot handle
--timeoutproperly. This is now fixed.