https://github.com/lukas-krecan/ShedLock/issues/366 added provider fo… #371
Conversation
|
Thanks a lot, I will take a look at it next week. |
There was a problem hiding this comment.
Thanks again. As I am not etcd expert, I have few questions
- Can you please document it. Especially please explain how the leases are used.
- If I understand the docs correctly, lease TTL is the minumum time, the server can decide to keep the lease longer. Can it really happen?
- In theory we could implement it without leases, just by using Transactions. The advantage of leases is the keep-alive mechanism, but the code without them might be easier. Did you consider it?
| throw new LockException("Can not remove node", e); | ||
| } | ||
| } else { | ||
| Long leaseId = etcdTemplate.createLease(keepLockFor); |
There was a problem hiding this comment.
Should be timed out and then removed, eventually.
| } else { | ||
| Long leaseId = etcdTemplate.createLease(keepLockFor); | ||
|
|
||
| etcdTemplate.updateLease(key, buildValue(), leaseId); |
There was a problem hiding this comment.
Does the method updateLease update lease or the value? Do we really want to update the value?
There was a problem hiding this comment.
the lease for the key is updated, similar to the CLI command etcdctl put foo2 bar2 --lease 694d76c7c330b909
There was a problem hiding this comment.
There is one missing answer, I owe you: whether or not we want to update the lease value.
Well, this should implement the lockAtLeast functionality. I checked the redis-jedis provider before starting this implementation, and it has a similar implementation AFAIK.
As always, I am open to any suggestions/improvements, though.
There was a problem hiding this comment.
I renamed the method to putWithLeaseId, because this is actually what the method does :-)
...lock-provider-etcd/src/main/java/net/javacrumbs/shedlock/provider/etcd/EtcdLockProvider.java
Outdated
Show resolved
Hide resolved
|
@lukas-krecan I am very open to simplify the code. What would be the idea to solve the timeout? |
I think it's better with leases. The alternative would be to do compare and swap either based on versions or value containing the lock validity. But let's keep the leases. |
|
I dug deeper to jetcd and if I understand it correctly, for keep-alive to work one has to call |
I called |
Yes, sure, I will. |
|
Thanks, I will look at it over the weekend. If you agree, I will do some minor cleanup. I feel it's time consuming to polish the code through PR comments, so I prefer to do minor changes myself. I plan to change the module name to something like |
Yes, sounds good. If you created the branch issue-366-etcd-support in your repository as well, I could switch the PR's target branch from master to issue-366-etcd-support. WDYT? Would this be a good option for you? |
|
I just planned to add commits on top of yours. The way I usually do it is that I create a new PR from your branch. So feel free to do whatever changes you want to this PR and over the weekend I will build on top of it. |
|
Released as 4.20.0 |
Implemented a provider for etcd like requested by @goudai in #366.
Note that etcds leases only support TTLs in seconds, therefore the integration tests needed some adjustments for this provider.