Skip to content

storage: fix lease not actually renewed#65393

Closed
llllash wants to merge 1 commit intocockroachdb:masterfrom
llllash:store-lease-bug
Closed

storage: fix lease not actually renewed#65393
llllash wants to merge 1 commit intocockroachdb:masterfrom
llllash:store-lease-bug

Conversation

@llllash
Copy link
Copy Markdown
Contributor

@llllash llllash commented May 18, 2021

i found that the replica lease_holder did not actually renew in function startLeaseRenewer().
I can't believe it's been around for three years...

Is my fix right?
review it plz, thanks~ @a-robinson @tbg

@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented May 18, 2021

CLA assistant check
All committers have signed the CLA.

@blathers-crl
Copy link
Copy Markdown

blathers-crl bot commented May 18, 2021

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

  • Please ensure your git commit message contains a release note.
  • When CI has completed, please ensure no errors have appeared.

I was unable to automatically find a reviewer. You can try CCing one of the following members:

  • A person you worked with closely on this PR.
  • The person who created the ticket, or a CRDB organization member involved with the ticket (author, commenter, etc.).
  • Join our community slack channel and ask on #contributors.
  • Try find someone else from here.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner labels May 18, 2021
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Contributor

@a-robinson a-robinson left a comment

Choose a reason for hiding this comment

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

Heh, it does actually look like this has been broken since #28975. Thanks for noticing!

I'll let @tbg handle commenting on the specifics of the fix, though.

tbg added a commit to tbg/cockroach that referenced this pull request May 25, 2021
We were never looping around more than once as a result of
never resetting the timer. This was noticed by contributor
@llllash in cockroachdb#65393. Thank you!

Closes cockroachdb#65393.

Release note: None
tbg added a commit to tbg/cockroach that referenced this pull request May 31, 2021
We were never looping around more than once as a result of
never resetting the timer. This was noticed by contributor
@llllash in cockroachdb#65393. Thank you!

Closes cockroachdb#65393.
Co-authored-by: linyimin <linyimin@baidu.com>

Release note: None
craig bot pushed a commit that referenced this pull request Jun 10, 2021
65653: kvserver: actually renew expiration-based leases r=nvanbenschoten a=tbg

We were never looping around more than once as a result of
never resetting the timer. This was noticed by contributor
@llllash in #65393. Thank you!

Closes #65393.

It seems as though the mechanism to prevent ping-ponging of the r1 lease never quite worked, i.e. this issue wasn't actually closed: #24753

It's unclear why this hasn't caused more trouble over the last three years. However, looking at the original issue, it seems like you need very precise timing to get the problem to appear. Maybe that is a good enough explanation.

Release note: None


Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
@craig craig bot closed this in f30e679 Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

O-community Originated from the community X-blathers-untriaged blathers was unable to find an owner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants