sql/logictest: add a directive to skip the rest of a logictest on retry#58217
Conversation
Some logictests, specifically schema_change_in_txn, periodically experience completely valid serializable restarts. We don't have a way to allow the test to proceed and retry in those cases. Instead, this allows the test to indicate that this happening is fine and to report instead that the test has been skipped. Touches cockroachdb#53724. Release note: None
|
This change is |
|
@jordanlewis how does this make you feel? |
jordanlewis
left a comment
There was a problem hiding this comment.
Interesting. Well, we'd never know if the entire test completely broke with this approach, would we? If we introduced a bug that caused these to always retry, we'd never notice - the test would just stay skipped.
I guess I prefer the approach I took in #56841, but I obviously never finished that...
|
We could look at the teamcity stats. It keeps track of skip counts. It’s better than skipping the whole test and a lot easier than tracking transaction state. |
jordanlewis
left a comment
There was a problem hiding this comment.
We could, but we probably won't. I'm happy to get this merged as it's going to alleviate random test failures.
But I just want to state for the record that I'm nervous that this will allow bugs to be swept under the rug. Ideally, we'd have a way to avoid that phenomenon. I guess trying to do better does really mean teaching logic test about txn state which is pretty brutal, but it may be worth it...
Alternatively, are we just using the wrong test framework for this sort of test? Do we need a different one that's more targeted to transactional workloads with retries?
Maybe, there's a lot in this test but I suppose it could be worth rewriting it. I'm leaving the issue open. TFTR! bors r=jordanlewis |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
When we run transactions in logictests, they can be exposed to retry errors. The framework does not have tools to retry whole blocks. In cockroachdb#58217 we added a mechanism to skip tests which hit such errors. Apply that logic here. (hopefully) Fixes cockroachdb#86215 Release note: None
85900: sql: implement `REPLACE FUNCTION` r=chengxiong-ruan a=chengxiong-ruan
This commit adds to the `REPLACE` path to the
`CREATE OR REPLACE FUNCTION` statement. Major changes are
(1) fetch function with same signature if exists, and validate
(2) remove refereces before replacing the function, and then
add new references.
Fixes #83236
Release note: None
Release justification: This commit includes a fix to avoid duplicate functions.
85964: externalconn: add gs support to External Connections r=rhu713 a=adityamaru
This change registers Google Storage `gs` as a supported
External Connection.
Release note (sql change): Users can now
`CREATE EXTERNAL CONNECTION` to represent an underlying
google storage resource.
Release justification: low risk change to new functionality around External Connections
86156: clusterversions: remove RowLevelTTL version r=rafiss a=rafiss
Release justification: low risk change to remove dead code
Release note: None
86179: roachtest: reduce slowness threshold in tpchvec/streamer r=yuzefovich a=yuzefovich
Release note: None
Release justification: test-only change.
86185: grunning: improve some tests r=irfansharif a=irfansharif
Release note: None
Release justification: test-only changes
86219: storage/metamorphic: skip TestPebbleEquivalence r=erikgrinaker a=jbowens
Skip TestPebbleEquivalence until #86102/#86088 is resolved.
Release note: None
Release justification: Non-production code changes.
86222: sql/logictest: attempt to deflake retry errors r=ajwerner a=ajwerner
When we run transactions in logictests, they can be exposed to retry errors.
The framework does not have tools to retry whole blocks. In #58217 we added
a mechanism to skip tests which hit such errors. Apply that logic here.
(hopefully)
Fixes #86215
Release justification: testing only change
Release note: None
86231: sqlsmith: skip crdb_internal.set_compaction_concurrency r=yuzefovich a=yuzefovich
Fixes: #86201
Release justification: test-only change.
Release note: None
Co-authored-by: Chengxiong Ruan <chengxiongruan@gmail.com>
Co-authored-by: Aditya Maru <adityamaru@gmail.com>
Co-authored-by: Rafi Shamim <rafi@cockroachlabs.com>
Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: irfan sharif <irfanmahmoudsharif@gmail.com>
Co-authored-by: Jackson Owens <jackson@cockroachlabs.com>
Co-authored-by: Andrew Werner <awerner32@gmail.com>
Some logictests, specifically schema_change_in_txn, periodically experience
completely valid serializable restarts. We don't have a way to allow the
test to proceed and retry in those cases. Instead, this allows the test to
indicate that this happening is fine and to report instead that the test has
been skipped.
Touches #53724.
Release note: None