multitenant: deflake TestConsumption #105552
Conversation
|
This change is |
There was a problem hiding this comment.
The original failure was on the 22.2 branch. I think you should add backport labels for 23.1 and 23.2.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)
pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go line 910 at r1 (raw file):
// Wait out the target period duration. time.Sleep(targetPeriod * 5)
Sleep can still be flaky, so it would be nice if we could find a way to not rely on it. Why not instead loop on waitForConsumption a few times if the checks on batches, requests, etc. haven't been met?
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go line 910 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
Sleep can still be flaky, so it would be nice if we could find a way to not rely on it. Why not instead loop on
waitForConsumptiona few times if the checks on batches, requests, etc. haven't been met?
This suggestion also has the potential to be flaky, but I think the sleep is worse, since when sleeping for a long time background tasks are also more likely to cause the checks to pass.
I might be missing some subtleties about how consumption works, though.
DrewKimball
force-pushed
the
test-consumption
branch
from
493e954 to
cc48906
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go line 910 at r1 (raw file):
Previously, rharding6373 (Rachael Harding) wrote…
This suggestion also has the potential to be flaky, but I think the sleep is worse, since when sleeping for a long time background tasks are also more likely to cause the checks to pass.
I might be missing some subtleties about how consumption works, though.
I changed to retry in a loop.
|
@rharding6373 friendly ping :) |
DrewKimball
added
backport-23.1.x
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go line 910 at r1 (raw file):
Previously, DrewKimball (Drew Kimball) wrote…
I changed to retry in a loop.
Instead of an explicit retry loop, why not use SucceedsSoon?
This patch adds retries to the `TestConsumption` test for retrieving the total tenant RU consumption in order to ensure that the delta includes the bucket flush that was triggered by the test query, instead of background activity. Fixes cockroachdb#94286 Fixes cockroachdb#106572 Release note: None
yuzefovich
force-pushed
the
test-consumption
branch
from
cc48906 to
d99cd3d
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:
pkg/ccl/multitenantccl/tenantcostclient/tenant_side_test.go line 910 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Instead of an explicit retry loop, why not use
SucceedsSoon?
I added SucceedsSoon, hope you don't mind Drew. I also stressed it on gceworker with no failures:
3324 runs so far, 0 failures, over 49m30s
Let's merge this to fix 2 issues :) @DrewKimball @rharding6373
SGTM, sorry I haven't gotten to this in a while. |
|
No worries! bors r+ |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
This patch adds retries to the
TestConsumptiontest for retrieving the total tenant RU consumption in order to ensure that the delta includes the bucket flush that was triggered by the test query, instead of background activity.Fixes #94286
Fixes #106572
Release note: None