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

storage: don't sync with reads on lease extension #8352

Merged

Conversation

andreimatei
Copy link
Contributor

@andreimatei andreimatei commented Aug 6, 2016

tschottdorf has a note in the code about why new leases need to
sometimes block reads while the lease is applied - you don't want reads
in between setting the new lease and updating the timestamp cache's
low-water mark. It seems safe to me to not do this in case of a lease
extension, since in that case we're not updating the aforementioned
low-water mark.
Besides hopefully being a perf optimization, this allows tests to do
lease extensions while a read is blocked, which I need for #8310.


This change is Reviewable

@andreimatei
Copy link
Contributor Author

cc @tschottdorf

@bdarnell
Copy link
Member

bdarnell commented Aug 7, 2016

:lgtm:


Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


storage/replica_command.go, line 1664 [r1] (raw file):

  ms *enginepb.MVCCStats,
  lease roachpb.Lease,
  isTransfer bool,

The name isTransfer suggests that this is only true for TransferLease, but that's not how it's used. Reverse this and call it isExtension?


Comments from Reviewable

tschottdorf has a note in the code about why new leases need to
sometimes block reads while the lease is applied - you don't want reads
in between setting the new lease and updating the timestamp cache's
low-water mark. It seems safe to me to not do this in case of a lease
extension, since in that case we're not updating the aforementioned
low-water mark.
Besides hopefully being a perf optimization, this allows tests to do
lease extensions while a read is blocked, which I need for cockroachdb#8310.
@andreimatei
Copy link
Contributor Author

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


storage/replica_command.go, line 1664 [r1] (raw file):

Previously, bdarnell (Ben Darnell) wrote…

The name isTransfer suggests that this is only true for TransferLease, but that's not how it's used. Reverse this and call it isExtension?

I didn't use `isExtension` because, at least to me, an "extension" sounds like it applies just when you have an active lease (as opposed to when the most recent lease is an expired one). But that's not how the word is used in the source, at lease in `RequestLease`, so ok, switching.

Comments from Reviewable

@andreimatei andreimatei merged commit d7b4983 into cockroachdb:master Aug 8, 2016
@andreimatei andreimatei deleted the lease-extension-no-trigger branch August 8, 2016 21:27
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.

None yet

2 participants