logictest: retry support for "statement" and "query" commands #96160
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @msirek)
pkg/sql/logictest/logic.go line 2547 at r1 (raw file):
case "retry": // retry is a standalone command that may precede a "statement" or "query"
Why is this a standalone command rather than a new option for statement command? To me having it both ways as an option to query and a standalone seems confusing.
nit: consider adding a quick comment in LogicTest language section at the top of this file.
pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 4 at r2 (raw file):
# LogicTest: multiregion-9node-3region-3azs !metamorphic-batch-sizes # TODO(msirek): Make this test work with the multiregion-9node-3region-3azs-vec-off config # Currently this test and other multiregion tests may flake when run under
nit: perhaps keep this as a part of the comment at the top of this file in order to explain why we need to add retry for many statements.
msirek
force-pushed
the
logictest_statement_retry
branch
from
f9e837f to
ba01528
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic.go line 2547 at r1 (raw file):
Why is this a standalone command rather than a new option for statement command?
Because "statement error" and "query error" statements are suffixed with an unlimited number of words, so nothing can come after them. The only other option would be to place "retry between "statement" and "error" like this: "statement retry error", which looks and sounds a little unnatural, and would make parsing of any future options to the "statement" directive more difficult to add (the developer would need to remember to explicitly parse the retry).
To me having it both ways as an option to query and a standalone seems confusing.
It's not required to use the standalone command. Do we really need to explicitly disable it for a "query" command which expects success? I suppose we could, but I think most people will just look at the current examples which are already used, e.g. query T retry and just copy them, so not be confused.
nit: consider adding a quick comment in LogicTest language section at the top of this file.
Done.
pkg/ccl/logictestccl/testdata/logic_test/multi_region_remote_access_error line 4 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: perhaps keep this as a part of the comment at the top of this file in order to explain why we need to add
retryfor many statements.
OK. Added:
# Currently this test and other multiregion tests may flake often when run
# under the multiregion-9node-3region-3azs-vec-off config, and less often
# under the multiregion-9node-3region-3azs config possibly due to zone
# configs not propagating in a timely manner after table creation. The `retry`
# directive and `retry` option to the `query` directive is used for a majority
# of the tests in this file to mitigate this issue, as cached zone configs are
# purged during the retry, forcing the cache to be repopulated with the proper,
# non-empty zone configs. See #87391.
| @@ -3849,6 +3876,7 @@ func (t *logicTest) formatValues(vals []string, valsPerLine int) []string { | |||
| } | |||
|
|
|||
| func (t *logicTest) success(file string) { | |||
| t.retry = false | |||
There was a problem hiding this comment.
What if success isn't called? I wonder if this should be set to false at the top of processSubtest to be safe.
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:
pkg/sql/logictest/logic.go line 2547 at r1 (raw file):
Previously, msirek (Mark Sirek) wrote…
Why is this a standalone command rather than a new option for statement command?
Because "statement error" and "query error" statements are suffixed with an unlimited number of words, so nothing can come after them. The only other option would be to place "retry between "statement" and "error" like this: "statement retry error", which looks and sounds a little unnatural, and would make parsing of any future options to the "statement" directive more difficult to add (the developer would need to remember to explicitly parse the retry).
To me having it both ways as an option to query and a standalone seems confusing.
It's not required to use the standalone command. Do we really need to explicitly disable it for a "query" command which expects success? I suppose we could, but I think most people will just look at the current examples which are already used, e.g.
query T retryand just copy them, so not be confused.nit: consider adding a quick comment in LogicTest language section at the top of this file.
Done.
Makes sense, thanks for the context.
pkg/sql/logictest/logic.go line 404 at r3 (raw file):
// - retry // Specifies that the next occurrence of a statement or query directive // (including those which expect errors), will be retried for a fixed
nit: the comma here is not needed I believe 😃
This adds a retry command to logic tests which may be issued on a separate line preceding either a "statement" or "query" command. It causes the statement to be retried with exponential backoff up to some maximum duration, e.g. ``` retry statement error column "non_exist" does not exist ALTER TABLE created_as_global SET LOCALITY REGIONAL BY ROW AS "non_exist" ``` This has the same effect as the retry option of the query command, but now also supports "statement ok", "statement error" and "query error" commands. Retry of a query command may be specified by the standalone retry command, the retry option of the query command, or both. Fixes: cockroachdb#95668 Epic: CRDB-20535 Release note: None
… test This enables the `multiregion-9node-3region-3azs-vec-off` config for the `multi_region_remote_access_error` test and adds retry commands prior to statements. Epic: CRDB-18645 Release note: None
msirek
force-pushed
the
logictest_statement_retry
branch
from
ba01528 to
e7ce7b9
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/logic.go line 2547 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Makes sense, thanks for the context.
np
pkg/sql/logictest/logic.go line 404 at r3 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: the comma here is not needed I believe 😃
Yeah, guess not. Removed it.
pkg/sql/logictest/logic.go line 3879 at r3 (raw file):
Previously, cucaroach (Tommy Reilly) wrote…
What if success isn't called? I wonder if this should be set to false at the top of processSubtest to be safe.
Good point. It's currently called after every awaitstatement, statement, awaitquery or awaitquery directive. But if we finish a subtest without calling one of these, it's probably good to reset t.retry to start fresh for the next subtest.
Made the change.
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
|
Build succeeded: |
logictest: retry support for "statement" and "query" commands
This commit adds a retry command to logic tests which may be issued on a
separate line preceding either a "statement" or "query" command.
It causes the statement to be retried with exponential backoff up
to some maximum duration, e.g.
This has the same effect as the retry option of the query command, but
now also supports "statement ok", "statement error" and "query error"
commands.
Retry of a query command may be specified by the standalone retry
command, the retry option of the query command, or both.
Fixes: #95668
Epic: CRDB-20535
Release note: None
logictest: vectorized off config for multi_region_remote_access_error test
This enables the
multiregion-9node-3region-3azs-vec-offconfig for themulti_region_remote_access_errortest and adds retry commands prior tostatements.
Epic: CRDB-18645
Release note: None