roachtest: increase consistency check timeout, and ignore errors #69285
Conversation
|
This change is |
There was a problem hiding this comment.
Should we bump it up a bit more though? It also seems as though we may want to not return an error if we hit the context cancellation. The consistency check is best-effort after all, and we don't want problems with it to post issues for an otherwise healthy test.
| @@ -1278,6 +1278,7 @@ WHERE t.status NOT IN ('RANGE_CONSISTENT', 'RANGE_INDETERMINATE')`) | |||
| l.Printf("consistency check failed with %v; ignoring", err) | |||
| return nil | |||
| } | |||
| defer rows.Close() | |||
There was a problem hiding this comment.
Nice. A quick check using the sqlclosecheck shows we have a few more cases of omitting a Close(). In many cases it likely doesn't matter because we are following up with t.Fatal(). (No need to handle them here, I'll open an issue)
erikgrinaker
force-pushed
the
roachtest-consistency-timeout
branch
from
10d516f to
9d7e56f
Compare
This bumps the consistency check timeout to 5 minutes. There are indications that a recent libpq upgrade unmasked previously ignored context cancellation errors, caused by the timeout here being too low. It also ignores errors during the consistency check, since it is best-effort anyway. Release justification: non-production code changes Release note: None
erikgrinaker
force-pushed
the
roachtest-consistency-timeout
branch
from
9d7e56f to
6b89733
Compare
erikgrinaker
changed the title
Sure, bumped to 5 minutes.
Makes sense, used the existing convention of outputting a warning for any errors. |
|
bors r=tbg |
|
Build succeeded: |
This bumps the consistency check timeout to 5 minutes. There are
indications that a recent libpq upgrade unmasked previously ignored
context cancellation errors, caused by the timeout here being too low.
It also ignores errors during the consistency check, since it is
best-effort anyway.
Resolves #68883.
Release justification: non-production code changes
Release note: None