kvserver: deflake TestProtectedTimestamps#110072
kvserver: deflake TestProtectedTimestamps#110072craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
Previously, we weren't waiting for the PTS record to make its way to KV in this test. This meant expecting it to provide protection (which the test asserted) was racy with spanconfig reconciliation. Span config reconciliation is built over rangefeeds, so when we bumped the default value of `kv.rangefeed.closed_timestamp_refresh_interval` in cockroachdb#108667, it meant span config reconciliation would take longer. This caused the GC queue to run before the replica was aware of the PTS record, which violated a test assertion. This patch fixes the issue by only enqueueing the range in the GC queue once the PTS record is known to have made it to KV. Closes cockroachdb#109244 Release note: None
|
This change is |
| ptsReader := tc.GetFirstStoreFromServer(t, 0).GetStoreConfig().ProtectedTimestampReader | ||
| require.NoError( | ||
| t, | ||
| ptutil.TestingVerifyProtectionTimestampExistsOnSpans( |
There was a problem hiding this comment.
Could you rename this helper to something that captures that there's a SucceedsSoon block internally? WaitUntil...? There's one usage that wraps this helper around a SucceedsSoon block as well, so it's confused at least one other person.
TestingWaitForProtectedTimestampToExistOnSpans captures the fact that there's a SucceedsSoon in there better. While here, we also clean up a few usages and no longer return a useless error. Epic: none Release note: None
arulajmani
left a comment
There was a problem hiding this comment.
TFTR!
bors r=irfansharif
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @pavelkalinnikov)
pkg/kv/kvserver/client_protectedts_test.go line 188 at r1 (raw file):
Previously, irfansharif (irfan sharif) wrote…
Could you rename this helper to something that captures that there's a SucceedsSoon block internally? WaitUntil...? There's one usage that wraps this helper around a SucceedsSoon block as well, so it's confused at least one other person.
Good call, tacked on a commit.
|
Build succeeded: |
Previously, we weren't waiting for the PTS record to make its way to KV in this test. This meant expecting it to provide protection (which the test asserted) was racy with spanconfig reconciliation. Span config reconciliation is built over rangefeeds, so when we bumped the default value of
kv.rangefeed.closed_timestamp_refresh_intervalin #108667, it meant span config reconciliation would take longer. This caused the GC queue to run before the replica was aware of the PTS record, which violated a test assertion.This patch fixes the issue by only enqueueing the range in the GC queue once the PTS record is known to have made it to KV.
Closes #109244
Release note: None