Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: move test to ccl #119434

Conversation

andrewbaptist
Copy link
Collaborator

Previously the test was validating the behavior with Global reads, however since the test was not in the ccl package it required complicated mangaling of the span configs. Now it simply uses the standard SQL commands to create the table.

Epic: none
Fixes: #119230

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@andrewbaptist andrewbaptist force-pushed the 2024-02-20-fix-global-reads-test branch 2 times, most recently from 0134529 to dea6bcd Compare February 21, 2024 02:04
@andrewbaptist andrewbaptist requested a review from pav-kv February 21, 2024 02:04
@andrewbaptist andrewbaptist marked this pull request as ready for review February 21, 2024 02:04
@andrewbaptist andrewbaptist requested review from a team as code owners February 21, 2024 02:04
Previously the test was validating the behavior with Global reads,
however since the test was not in the ccl package it required
complicated mangaling of the span configs.  Now it simply uses the
standard SQL commands to create the table.

Epic: none
Fixes: cockroachdb#119230

Release note: None
@andrewbaptist andrewbaptist force-pushed the 2024-02-20-fix-global-reads-test branch from dea6bcd to efc85ce Compare February 26, 2024 20:47
Copy link
Member

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change. It doesn't feel like pkg/ccl/multiregionccl is the right home for a test that's primarily interested in the behavior of a function (maybeCommitWaitBeforeCommitTrigger) deep inside the pkg/kv/kvserver package.

Was the way we were configuring this the cause of the flakiness? Is there a better way that we can configure spanconfigs from within the kvserver package without passing through SQL?

Reviewed 1 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @pav-kv)

@andrewbaptist
Copy link
Collaborator Author

I agree with your reluctance here, I didn't like this much either, and I wouldn't have put it there except that its not possible to create a table with global locality without it being in CCL.

The problem is due to the way that span configs today can only be manually set up through SQL and worse that we can only set the span configs on replicas. We could hold off on merging this, but it will require a fairly major change to the way we create span configs to do this correctly. I have some thoughts about how to do this, but the change is somewhat invasive. Specifically we would want to disable either the automatic span config creation or alternatively some type of manual SpanConfigSubscriber to get around this limitation.

I had also considered creating a ccl package within kvserver which was only for tests that required the ccl.

From a very long term perspective, we should draw a stronger line and not have KV depend on SQL at all, even for unit tests. That will require a lot of work to happen given where we are now. SpanConfigs particularly are at an ugly border.

@rafiss rafiss removed the request for review from a team March 13, 2024 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kv/kvserver: TestStoreRangeSplitAndMergeWithGlobalReads failed
3 participants