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

config/sql: add global_reads field to ZoneConfig, use for GLOBAL tables #59304

Merged

Conversation

nvanbenschoten
Copy link
Member

@nvanbenschoten nvanbenschoten commented Jan 22, 2021

Closes #57689.

This PR adds a new global_reads field to the ZoneConfig struct. global_reads specifies whether transactions operating over the range(s) should be configured to provide non-blocking behavior, meaning that reads can be served consistently from all replicas and do not block on writes. In exchange, writes get pushed into the future and must wait on commit to ensure linearizability. For more, see: https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20200811_non_blocking_txns.md

The PR then enables global_reads for tables with the GLOBAL locality config. global_reads are not yet hooked up in KV because we don't yet have per-range closed timestamp tracking, but this completes the SQL-level work for GLOBAL tables, with the exception of #57663, which will add non-voting replicas (at the database zone config level) for all database regions that don't already have a voting replica.

Release note (sql change): A new, unused field called "global_reads" was added to zone configurations. The field does not yet have any effect.

@nvanbenschoten nvanbenschoten requested review from ajstorm and a team January 22, 2021 17:42
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@ajstorm ajstorm left a comment

Choose a reason for hiding this comment

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

Looks great.

Super minor nit: While this doesn't expose the functionality of non-blocking transactions, it does make the fields visible for introspection. As a result, it might warrant a release note as alpha customers could see this field and be confused.

Reviewed 10 of 10 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@nvanbenschoten
Copy link
Member Author

Super minor nit: While this doesn't expose the functionality of non-blocking transactions, it does make the fields visible for introspection. As a result, it might warrant a release note as alpha customers could see this field and be confused.

That's fair. I'll add one.

I also wanted to get your opinion on something. I'm starting to think that non_blocking_txns is not the right external-facing name for this functionality. It's the (or "a") right name for the underlying technology, but now that we're talking about what we actually expose to users, I don't think it's all that helpful. It's too much of an implementation detail and doesn't actually speak about the primary benefit of configuring such a range.

Now that we've resolved the implementation dispute of #52745 vs. #39758 (which I need to merge as "rejected"), I'm thinking about stealing some terminology from the original RFC. Specifically, I'm thinking about renaming this field in the zone configs to consistent_read_replicas or consistent_followers (both in contrast to "stale" followers). Does that resonate at all with you?

Copy link
Collaborator

@ajstorm ajstorm left a comment

Choose a reason for hiding this comment

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

Yes, totally. The one caveat I'd point out with each of those is that (to me at least) both of them imply that they would take a numeric setting. Other names we might want to consider: consistent_local_reads, fast_consistent_reads.

Are we only going to allow this to be enabled on GLOBAL tables (or tables which are manually configured to be in multiple regions)?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained

@nvanbenschoten
Copy link
Member Author

The one caveat I'd point out with each of those is that (to me at least) both of them imply that they would take a numeric setting.

That's a good point. They do sound numeric. I see where you're going with your two suggestions. My only hesitation to them is that they're introducing new terminology that we don't use elsewhere, so they're harder to pattern match against. Since we tend to use "follower read" instead of "local read" everywhere, how about consistent_follower_reads?

Are we only going to allow this to be enabled on GLOBAL tables (or tables which are manually configured to be in multiple regions)?

I think we're going to allow this to be enabled on tables with manual zone configurations. That may be going against our intention to restrict this to enterprise though. If so, I think we'd ideally block the field for non-enterprise users. But we don't have any precedent for that at the zone config level and adding it seems tough. The easier thing to do would be to simply not route to followers for consistent follower reads in non-enterprise deployments, just like we already don't route to followers for stale follower reads in non-enterprise deployments.

Closes cockroachdb#57689.

This commit adds a new global_reads field to the ZoneConfig struct.
global_reads specifies whether transactions operating over the range(s)
should be configured to provide non-blocking behavior, meaning that
reads can be served consistently from all replicas and do not block on
writes. In exchange, writes get pushed into the future and must wait on
commit to ensure linearizability. For more, see:
https://github.com/cockroachdb/cockroach/blob/master/docs/RFCS/20200811_non_blocking_txns.md

Release note (sql change): A new, unused field called "global_reads" was
added to zone configurations. The field does not yet have any effect.
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/zoneConfigNonBlocking branch from cf12dae to c7bbb1e Compare January 27, 2021 01:21
@nvanbenschoten nvanbenschoten changed the title config/sql: add non_blocking_txns field to ZoneConfig, use for GLOBAL tables config/sql: add global_reads field to ZoneConfig, use for GLOBAL tables Jan 27, 2021
@nvanbenschoten
Copy link
Member Author

Super minor nit: While this doesn't expose the functionality of non-blocking transactions, it does make the fields visible for introspection. As a result, it might warrant a release note as alpha customers could see this field and be confused.

Done.

I ended up updating this field name to global_reads. This seemed to be the name that most people liked due to its synergy with GLOBAL tables and because it avoided any confusion with the term "follower reads", which has too strong of an implied connection to "stale reads" to be worth re-using/extending. There was a bit of discussion about whether global_reads=false implies that you can't read from any other region, but this doesn't seem any worse than the implication that a REGIONAL table, as opposed to a GLOBAL table, implies that you can't read from any other region, so I decided to proceed with this name.

This commit enables global_reads for tables with the GLOBAL locality
config. global_reads are not yet hooked up in KV because we don't yet
have per-range closed timestamp tracking, but this completes the
SQL-level work for GLOBAL tables, with the exception of cockroachdb#57663, which
will add non-voting replicas (at the database zone config level) for all
database regions that don't already have a voting replica.

Release note: None
@nvanbenschoten nvanbenschoten force-pushed the nvanbenschoten/zoneConfigNonBlocking branch from c7bbb1e to 91fbbca Compare January 27, 2021 05:26
@nvanbenschoten
Copy link
Member Author

TFTR!

bors r=ajstorm

@craig
Copy link
Contributor

craig bot commented Jan 27, 2021

Build succeeded:

@craig craig bot merged commit 9a93daf into cockroachdb:master Jan 27, 2021
@nvanbenschoten nvanbenschoten deleted the nvanbenschoten/zoneConfigNonBlocking branch February 24, 2021 04:32
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: introduce non-blocking ranges zone configs
3 participants