streamingccl: deflake TestRandomClientGeneration #100813
Conversation
|
This change is |
msbutler
force-pushed
the
butler-c2c-TestClient
branch
from
d9a09f6 to
4f0508a
Compare
msbutler
requested review from
benbardin and
adityamaru
and removed request for
a team
msbutler
added
T-disaster-recovery
backport-23.1.x
| } | ||
| require.Equal(t, true, foundKVs, "expected to find and assert equal kvs") |
There was a problem hiding this comment.
nit: could use require.True
| targetSpan := roachpb.Span{Key: startKey, EndKey: startKey.PrefixEnd()} | ||
| if assertEqualKVs(t, srv, streamValidator, targetSpan, | ||
| maxResolvedTimestampPerPartition[pSpan]) { | ||
| foundKVs = true |
There was a problem hiding this comment.
don't we want to assert equal kvs for each iteration of this loop? IIUC the way this is structured even if one partition has equal KVs but the others don't the test will pass, cause foundKVs will be true.
There was a problem hiding this comment.
ah good q, under stress-race, it's possible for one partition to produce no checkpoint events. The stream ingestion processor only shuts down after 1000 checkpoint events were recorded, which can all occur from a single partition. I'll add a comment about this.
There was a problem hiding this comment.
I see, but how will we differentiate actual key mismatches vs partitions not producing any checkpoint events? Will something fatal in assertEqualKVs?
There was a problem hiding this comment.
the function returns true if it validated at least one kv. So if there are no checkpoints, it will not validate any kvs and return false. If it surfaces any kvs, they will be validated with assertions. Added a bit more the func docstring to reflect this.
msbutler
force-pushed
the
butler-c2c-TestClient
branch
from
4f0508a to
516c9c2
Compare
| targetSpan := roachpb.Span{Key: startKey, EndKey: startKey.PrefixEnd()} | ||
| if assertEqualKVs(t, srv, streamValidator, targetSpan, | ||
| maxResolvedTimestampPerPartition[pSpan]) { | ||
| foundKVs = true |
There was a problem hiding this comment.
I see, but how will we differentiate actual key mismatches vs partitions not producing any checkpoint events? Will something fatal in assertEqualKVs?
This patch fixes 4 bugs in TestRandomClientGeneration that were responsible for the persistent flakiness and lack of coverage in this test: - the randomeStreamClient no longer instantiates keys with a table prefix that collides with the job info table prefix. This collision was the original cause of the flakes reported in cockroachdb#99343. - getPartitionSpanToTableId() now generates a correct map from source partition key space to table Id. Previously, the key spans in the map didn't contain keys that mapped to anything logical in the cockroach key space. - assertKVs() now checks for keys in the destination tenant keyspace. - assertKVs() now actually asserts that kvs were found. Before, the assertion could pass if no keys were actually checked, which has been happening for months and allowed the bugs above to infest this test. Fixes cockroachdb#99343 Release note: None
msbutler
force-pushed
the
butler-c2c-TestClient
branch
from
516c9c2 to
3539505
Compare
|
TFTR! bors r=adityamaru |
|
Build succeeded: |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 3539505 to blathers/backport-release-23.1-100813: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch 23.1.x failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
This patch fixes 4 bugs in TestRandomClientGeneration that were responsible for the persistent flakiness and lack of coverage in this test:
Fixes #99343
Release note: None