Skip to content

Conversation

@aerfrei
Copy link
Contributor

@aerfrei aerfrei commented Aug 28, 2025

Previously, this test would flake because a table that we were asserting the admin user could see hadn't yet become visible. Now we assert the changefeed and rangefeed have started by waiting for a highwater mark.

Epic: none
Fixes: #152060

Release note: None

@aerfrei aerfrei requested a review from a team as a code owner August 28, 2025 20:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@andyyang890 andyyang890 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find!

{`regularUser`, false},
{`adminUser`, true},
{`viewClusterMetadataUser`, true},
{`feedCreator`, false},
Copy link
Collaborator

@andyyang890 andyyang890 Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of reordering the cases? I ask because changes like this have the cost of adding another commit to the git blame history (for these lines) so there needs to be some kind of benefit that outweighs that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that the feed creator case was possible not because they lacked the permissions but because no one was able to see the table since the rangefeed hadn't started up yet (this is the reason the admin user case was failing). Putting the true cases before the false cases ensures that the tables are visible to someone when the feed creator can't see them. Do you think it's worth including? I don't feel strongly

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it's not really worth switching since with either order, the subtest that would fail would still be adminUser right? We might actually want to randomize the order by using a map instead of a slice so there's more variance, up to you if you want to make that change

Previously, this test would flake because a table that we
were asserting the admin user could see hadn't yet become
visible. Now we assert the changefeed and rangefeed have
started by waiting for a highwater mark.

Epic: none
Fixes: cockroachdb#152060

Release note: None
@aerfrei aerfrei force-pushed the group-test-failure branch from eed0d7a to 9f48bed Compare August 29, 2025 15:51
@aerfrei
Copy link
Contributor Author

aerfrei commented Sep 2, 2025

I believe that between this and #152850 (merging now) that this test should pass. The extended CI failure should be fixed by #152850.

@aerfrei
Copy link
Contributor Author

aerfrei commented Sep 2, 2025

bors r=andyyang890

@craig
Copy link
Contributor

craig bot commented Sep 2, 2025

@craig craig bot merged commit 0d92408 into cockroachdb:master Sep 2, 2025
22 of 23 checks passed
@aerfrei
Copy link
Contributor Author

aerfrei commented Oct 9, 2025

blathers backport 24.3 25.2 25.3

@blathers-crl
Copy link

blathers-crl bot commented Oct 9, 2025

Based on the specified backports for this PR, I applied new labels to the following linked issue(s). Please adjust the labels as needed to match the branches actually affected by the issue(s), including adding any known older branches.


Issue #152060: branch-release-24.3, branch-release-25.2, branch-release-25.3.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ccl/changefeedccl: TestDistSenderRangeFeedPopulatesVirtualTable failed

3 participants