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.

Sign up for GitHub

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

Jump to bottom

kvclient/rangefeed,lease: introduce library for rangefeeds and adopt #58361

Closed
wants to merge 2 commits into from
Closed

kvclient/rangefeed,lease: introduce library for rangefeeds and adopt #58361

wants to merge 2 commits into from

Conversation

ajwerner
Copy link
Contributor

See individual commits.

@ajwerner ajwerner requested a review from a team as a code owner December 30, 2020 06:24
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner mentioned this pull request Dec 30, 2020
Merged
@ajwerner
Copy link
Contributor Author

@HonoreDB I'm tagging you because I think this code realistically ought to become one with the scanning and rangefeed logic in changefeedccl/kvfeed. They differ in their lifecycle requirements, frontier interaction, and the need to support multiple spans. Otherwise it's pretty much the same thing. Perhaps the way the scanning and rangefeeds are hooked together isn't exactly ideal. I think this has real value regardless of getting it to the point of unification, just putting it out there that that might be a goal.

@ajwerner ajwerner force-pushed the ajwerner/rangefeed-lib branch 5 times, most recently from 5390354 to 071bdc2 Compare December 30, 2020 16:54
@ajwerner
kvclient/rangefeed: introduce package for using rangefeeds
7a7577b 
This commit introduces a new package underneath kvclient to simplify
using rangefeeds. Namely, it provides sane retry and shutdown logic as well
as support for performing an initial data scan. It also hides the nasty
unwrapping of the DistSender as performed elsewhere.

A longer-term vision here would be to support multiple spans and utilize this
layer in the `kvfeed` of `changefeedccl`.

Release note: None
@ajwerner
server,lease: adopt new rangefeed package
197e527 
Release note: None
@nvanbenschoten
Copy link
Member

@tbg and I left comments on #58362, which includes these two commits.

@RaduBerinde RaduBerinde mentioned this pull request Feb 10, 2021
Closed
craig bot pushed a commit that referenced this pull request Feb 18, 2021
@ajwerner @dt @nvanbenschoten
Merge #58362 #59571 #60021
ca1a1a7 
58362: *: craft rangefeed abstraction, support cluster settings in tenants r=nvanbenschoten a=ajwerner

This PR is a reworking of @dt's excellent effort in #57926
to pick up rangefeeds for tenant cluster settings after sprucing up the libraries around
them. @tbg and @nvanbenschoten I'm tagging you accordingly given that you reviewed
that PR.

### First two commits (`kvclient/rangefeed` package and adoption in `catalog/lease`)

The first commit in this PR abstract out some rangefeed client logic around
retries to make them more generally usable in other contexts. The second
commit adopts this package in `sql/catalog/lease`. These two commits can
(and probably should) be merged separately and exist in
#58361.

### Next two commits (`server/settingswatcher` package and adoption in `server`)

These commits introduce code to update the settings using the above rangefeed
package and then hooks it up to the sql server.

### Next three commits

Remove the restrictions from tenants updating settings, mark which settings can be
updated, generate docs. 

These are lifted straight from @dt in #57926.

Maybe closes #56623.


59571: kv: route present-time reads to global_read follower replicas r=nvanbenschoten a=nvanbenschoten

This commit updates the kv client routing logic to account for the new `LEAD_FOR_GLOBAL_READS` `RangeClosedTimestampPolicy` added in #59505. In enterprise builds, non-stale read-only requests to ranges with this closed timestamp policy can now be routed to follower replicas, just like stale read-only requests to normal ranges currently are.

In addition to this main change, there are a few smaller changes in this PR that were hard to split out, so are included in this commit.

First, non-transactional requests are no longer served by followers even if the follower replica detects that the request can be. Previously, non-transactional requests would never be routed intentionally to a follower replica, but `Replica.canServeFollowerRead` would allow them through if their timestamp was low enough. This change was made in order to keep the client and server logic in-sync and because this does not seem safe for non-transactional requests that get their timestamp assigned on the server. We're planning to remove non-transactional requests soon anyway (#58459), so this isn't a big deal. It mostly just caused some testing fallout.

Second, transactional requests that had previously written intents are now allowed to have their read-only requests served by followers, as long as those followers have a closed timestamp above the transaction's read *and* write timestamp. Previously, we had avoided this because it seemed to cause issues with read-your-writes. However, it appears safe as long as the write timestamp is below the closed timestamp, because we know all of the transaction's intents are at or below its write timestamp. This is very important for multi-region work, where we want a transaction to be able to write to a REGIONAL table and then later perform local reads (maybe foreign key checks) on GLOBAL tables.

Third, a max clock offset shift in `canUseFollowerRead` was removed. It wasn't exactly clear what this was doing. It was introduced in the original 70be833 and seemed to allow a follower read in cases where they otherwise shouldn't be expected to succeed. I thought at first that it was accounting for the fact that the kv client's clock might be leading the kv server's clock and so it was being pessimistic about the expected closed timestamp, but it actually seems to be shifting the other way, so I don't know. I might be missing something.

Finally, the concept of a `replicaoracle.OracleFactoryFunc` was removed and the existing `replicaoracle.OracleFactory` takes its place. We no longer need the double-factory approach because the transaction object is now passed directly to `Oracle.ChoosePreferredReplica`. This was a necessary change, because the process of determining whether a follower read can be served now requires transaction information and range information (i.e. the closed timestamp policy), so we need to make it in the Oracle itself instead of in the OracleFactory. This all seems like a simplification anyway.

This is still waiting on changes to the closed timestamp system to be able to write end-to-end tests using this new functionality.

60021: roachpb: use LocalUncertaintyLimit from err in readWithinUncertaintyIntervalRetryTimestamp r=nvanbenschoten a=nvanbenschoten

Fixes #57685.

This commit updates `readWithinUncertaintyIntervalRetryTimestamp` to use the the `LocalUncertaintyLimit` field from `ReadWithinUncertaintyIntervalError` in place of `ObservedTimestamps`. This addresses a bug where `ReadWithinUncertaintyIntervalErrors` thrown by follower replicas during follower reads would use meaningless observed timestamps.

Co-authored-by: Andrew Werner <ajwerner@cockroachlabs.com>
Co-authored-by: David Taylor <tinystatemachine@gmail.com>
Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
@ajwerner
Copy link
Contributor Author

Merged as part of #58362.

@ajwerner ajwerner closed this Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nvanbenschoten nvanbenschoten Awaiting requested review from nvanbenschoten

@HonoreDB HonoreDB Awaiting requested review from HonoreDB

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ajwerner @cockroach-teamcity @nvanbenschoten