logictest: swap directives with default config when possible #52680
Conversation
|
This change is |
|
Hmm, we probably do not want to do this for the |
There was a problem hiding this comment.
I'm worried that we'll be hitting 30 minutes timeout more often with this change. In particular, fk file is huge, so I think it deserves special consideration about which configs to use when running it (all others are quite small, so all default configs should be ok in there).
I think if the file has explicit logic to split and relocate ranges assuming 5 node clusters, then those should not be updated.
Reviewed 35 of 35 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @asubiotto and @nvanbenschoten)
pkg/sql/logictest/testdata/logic_test/edge, line 2 at r1 (raw file):
# LogicTest: local-vec-off fakedist-vec-off # TODO(yuzefovich): run with all default configs once #40354 is resolved.
Is this no longer an issue? I wonder what's changed because the issue is still present, I think.
pkg/sql/logictest/testdata/logic_test/window, line 1 at r1 (raw file):
# LogicTest: default-configs 5node-default-configs
By deleting this line we will no longer run this test on 5node configs. However, this file doesn't split and relocate ranges, so probably it's ok.
There was a problem hiding this comment.
I'm worried that we'll be hitting 30 minutes timeout more often with this change. In particular, fk file is huge, so I think it deserves special consideration about which configs to use when running it (all others are quite small, so all default configs should be ok in there).
I'm not worried. I prefer running more tests with the default config than trying to stay under a timeout. If the runtime of the logic tests is a problem then I think that should be dealt with separately. The reasons I'm not worried about it is:
- We removed
3node-tenantfrom the default config, removing one run from all files run with the default config so we have breathing space. - Pretty soon we'll be reducing the number of default configs (at least the
autoandauto-diskconfigs). - If test runtime does become a problem and we want to solve it by running fewer tests, I prefer a policy of exclusion (i.e. blocking configs we don't see much use in running) to get automatic coverage of additions to the default configs.
I'll take a look at the TestLogic runtime on a green run of this PR.
I think if the file has explicit logic to split and relocate ranges assuming 5 node clusters, then those should not be updated.
I ran all these files with the new configurations and automatically updated the configs if it ran successfully. I think that these kinds of tests would fail with a non-5node config, so shouldn't be updated.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/distsql_join, line 1 at r1 (raw file):
# LogicTest: 5node-default-configs
What do you think of this removal @yuzefovich? Was there any good reason to use a 5node config here?
pkg/sql/logictest/testdata/logic_test/edge, line 2 at r1 (raw file):
Previously, yuzefovich wrote…
Is this no longer an issue? I wonder what's changed because the issue is still present, I think.
Yep, the test ran successfully. Maybe post-processing delegation to the row engine?
pkg/sql/logictest/testdata/logic_test/interleaved_join, line 1 at r1 (raw file):
# LogicTest: !local-spec-planning(47473) !fakedist-spec-planning(47473)
@yuzefovich OOC what was the reason for blocking this test?
pkg/sql/logictest/testdata/logic_test/window, line 1 at r1 (raw file):
Previously, yuzefovich wrote…
By deleting this line we will no longer run this test on 5node configs. However, this file doesn't split and relocate ranges, so probably it's ok.
Done.
A while back, we added the ability to run logic tests without a directive, which would run all default configs. Some tests still specified explicit configs even though they could run without a problem with all default configs. Even though it takes more time, running with all default configs is preferable, since developers rely on the widespread usage of the default config for good test coverage of new features. Additionally, all files affected can also be run in a multitenant configuration using 3node-tenant. So that TestTenantLogic may run these files, this commit adds a blocklist directive with an issue number where appropriate for any tests that fail in 3node-tenant that previously had explicit directives. Release note: None (testing change)
|
|
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/distsql_join, line 1 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
What do you think of this removal @yuzefovich? Was there any good reason to use a
5nodeconfig here?
I don't see a good reason. I think this file has had 5node configs since its introduction in 2017.
pkg/sql/logictest/testdata/logic_test/edge, line 2 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
Yep, the test ran successfully. Maybe post-processing delegation to the row engine?
I don't think so. I'll take a look at why this isn't failing anymore.
pkg/sql/logictest/testdata/logic_test/interleaved_join, line 1 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
@yuzefovich OOC what was the reason for blocking this test?
When I added support for the joins in the new factory, interleaved joins were planned in an ad-hoc manner (there was no separate ConstructInterleavedJoin method, instead, hash or merge joins could be converted into interleaved joins). As a result, the new factory was planning hash or merge joins and not interleaved ones as the test expects. Since then Radu has cleaned things up, and now the new factory correctly falls back to the old path when an interleaved join can be planned.
|
TFTR bors r=yuzefovich |
|
Build succeeded: |
|
|
52765: logictest: add 3node-tenant directive to logictests with non-default configs r=yuzefovich,nvanbenschoten a=asubiotto This commit is the result of running all logictests that specify non-default configs after applying #52680, and adding the 3node-tenant directive to tests that pass. For all tests with non-default configs that fail with the 3node-tenant configuration, the failures were analyzed and marked as either expected (no action necessary) or unexpected, in which case a 3node-tenant blocklist directive with an associated issue was added to the list of directives. Release note: None (testing change) Closes #52410 Co-authored-by: Alfonso Subiotto Marques <alfonso@cockroachlabs.com>
A while back, we added the ability to run logic tests without a directive,
which would run all default configs.
Some tests still specified explicit configs even though they could run without
a problem with all default configs. Even though it takes more time, running
with all default configs is preferable, since developers rely on the widespread
usage of the default config for good test coverage of new features.
Additionally, all files affected can also be run in a multitenant configuration
using 3node-tenant. So that TestTenantLogic may run these files, this commit
adds a blocklist directive with an issue number where appropriate for any tests
that fail in 3node-tenant that previously had explicit directives.
Release note: None (testing change)
Addresses #52410