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

Should not update uuid in old held leases #3

Open
cjorba opened this issue Dec 15, 2017 · 3 comments
Open

Should not update uuid in old held leases #3

cjorba opened this issue Dec 15, 2017 · 3 comments

Comments

@cjorba
Copy link

cjorba commented Dec 15, 2017

We've found issues when trying to update the lease with extra fields when it has been renewed by the renewer gorutine. Looking into it we've found that it is because the concurrencyToken is being updated (along with the whole Lease struct) every time it is renewed.
By comparing it with the base Java implementation I think the concurrencyToken should not change on Renew, only in Take.
In the commit history we've seen you already had fixed this on commit 5737f37 yet you changed your mind on commit e663b4a; I was wondering if you could elaborate on why this last commit was needed.

If you need more info or want to comment about it please don't hesitate to contact me.

Greetings

@a8m
Copy link
Owner

a8m commented Dec 23, 2017

Hey @cjorba, Thanks for taking the time to open this issue, and sorry for the delay. I was a bit busy lately.
The commit message is a bit poor (I need to work on that), but I think I added it because of the ForceUpdate method.
After investigating it, I quite sure that the problem is here. I'm generating a new concurrency-token each update, instead of reusing the existing one.
I'll planning to fix that, revert the commit you mentioned, and add integration tests for this package.

I'll continue to update this ticket.

@cjorba
Copy link
Author

cjorba commented Jan 2, 2018

Hi, sorry for the delay,
I think the Decode function it's ok as it is now. I think the only change that is needed is to revert commit e663b4a.
I can do the pull request for you to review.

@cjorba
Copy link
Author

cjorba commented Jan 3, 2018

I've created this Pull Request: #4
Besides putting back the missing if in Renew I've added a line in RenewLease that updates lastRenew along with the Counter.

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

No branches or pull requests

2 participants