sql: deflake TestTrace#99062
Conversation
|
This change is |
8bc7255 to
6aa3eb6
Compare
rharding6373
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker)
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:
-- commits line 16 at r1:
This suggests that the flake was possible before but it was made more likely recently, right?
pkg/sql/trace_test.go line 385 at r1 (raw file):
t.Errorf("extra span: %s", op) remainingErr = true } else if op != test.expSpans[r] {
I wonder whether we should change this comparison from exact equality to strings.Contains and perhaps also remove storage.Store prefix from one of other always optional spans.
|
Would this fix #98971? cc @abarganier |
|
Yes. |
This has been seen to flake in CI:
```
=== RUN TestTrace/ShowTraceForVectorized/TracingOff/node-1
trace_test.go:386: expected span: "session recording", got: "pendingLeaseRequest: requesting lease"
trace_test.go:397: remaining span: "session recording"
trace_test.go:397: remaining span: "sql query"
trace_test.go:397: remaining span: "sql txn"
trace_test.go:397: remaining span: "txn coordinator send"
--- FAIL: TestTrace/ShowTraceForVectorized/TracingOff/node-1 (0.02s)
```
There was already an exception for this span, but with a `storage.`
prefix. This patch removes the prefix, and makes it match on substrings.
This flake has possibly been made worse with the introduction of a
metamorphic setting to only use expiration-based leases in ecc931b.
Epic: none
Release note: None
6aa3eb6 to
c7641af
Compare
erikgrinaker
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, yuzefovich (Yahor Yuzefovich) wrote…
This suggests that the flake was possible before but it was made more likely recently, right?
No idea, but presumably the ignore rule is there for a reason.
pkg/sql/trace_test.go line 385 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
I wonder whether we should change this comparison from exact equality to
strings.Containsand perhaps also removestorage.Storeprefix from one of other always optional spans.
Yeah, may as well, done. Let's see what CI says.
|
I'm calling it a night -- feel free to bors this for me on green CI. |
yuzefovich
left a comment
There was a problem hiding this comment.
bors r+
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
abarganier
left a comment
There was a problem hiding this comment.
Reviewable status:
|
I see this was merged 7 hours ago - a fix to increase the timeout with a specific mention of I'm going to check to see if this SHA is included on this branch. If not, I'll push a rebase. |
|
No need to push the rebase to this branch - bors automatically picks up the newest changes and rebases on top of them the batch that it attempts to merge. Thus, although the CI is red on this PR (because this branch doesn't contain the fix), but the fix has already been merged, then the CI run when borsing should succeed. |
|
@yuzefovich I see, thanks for clarifying. I didn't realize a |
|
Just to clarify a bit further - the way bors works is as follows:
With such setup it doesn't matter whether CI runs on the PRs on their own were successful or not (they are not marked as "required"). (The only required are currently two things: 1) there is at least one GitHub approval on the PR, and 2) the PR is "borsed".) |
|
Build succeeded: |
This has been seen to flake in CI:
There was already an exception for this span, but with a
storage.prefix. This patch removes the prefix, and makes it match on substrings.This flake has possibly been made worse with the introduction of a metamorphic setting to only use expiration-based leases in ecc931b.
Resolves #98971.
Epic: none
Release note: None