GH-3444: Add Custom TTL support for RedisLock, and JdbcLock #9053
Conversation
There was a problem hiding this comment.
Thanks for looking into this!
I still think it has to be called a DistributedLock from the beginning: we may come up eventually with other API, not only TTL.
I think the LockRegistry must be modified for a generic arg: LockRegistry<L extends Lock>.
And no need in the extra CustomTtlLockRegistry abstraction.
Can you elaborate, please, why do we need a result from delete()?
Since this is a huge braking change (as it is expected according to the discussion in the issue), we are going to look into this in details in the next 6.4 version, which we are going to start this June.
If the result(row count) is 0, which means that the lock is not owned by current process. For example: In this case, an IllegalStateException should be thrown to info the process that the integrity of data protected by this lock may have been compromised. |
| } | ||
| catch (TransientDataAccessException | TransactionTimedOutException | TransactionSystemException e) { | ||
| // try again | ||
| } | ||
| catch (IllegalStateException e) { | ||
| throw new IllegalStateException("Lock was released in the store due to expiration. " + |
There was a problem hiding this comment.
OK. I see why you use boolean for delete() now.
I wonder if we can use a ConcurrentModificationException instead.
Or something from the org.springframework.dao package, like ConcurrencyFailureException
There was a problem hiding this comment.
RedisLock throw IllegalStateException when it face this scenario.
Should we also modify the implementation of RedisLock?
There was a problem hiding this comment.
I see. Well, that was just a suggestion if that is makes sense to rely on some specific exception instead of this common.
Might be the fact that ConcurrentModificationException does not fit in our locks scenario.
However that ConcurrencyFailureException might be OK.
There was a problem hiding this comment.
catch (TransientDataAccessException | TransactionTimedOutException | TransactionSystemException e) {
// try again
}
In the current implementation, TransientDataAccessException, TransactionTimedOutException, and TransactionSystemException will be caught, and then the delete() method will be invoked again.
Since ConcurrencyFailureException extends TransientDataAccessException, I think we should not throw it for this scenario.
I will change it to throw ConcurrentModificationException instead if you think ConcurrentModificationException fits better.
There was a problem hiding this comment.
I see. Thanks. Missed that.
So, yeah, this ConcurrentModificationException fits in my brain because its name means exactly what happens here for us.
But I'm not sure if this is a correct scenario where we can use it from Java perspective.
That's why I'm reaching you for other opinion, arguments and so on.
Just want to be sure that we are on the same page with a decision to move on.
There was a problem hiding this comment.
I see,
I agree that changing from IllegalStateException to ConcurrentModificationException can improve the readability.
But I'm also not sure if this is a correct scenario where we can use it from Java perspective.
How about we create our own exception -LockOwnershipExpiredException for Spring distributed locks?
It can be used by both RedisLock, and JdbcLock.
|
This is good stuff. As I expected this is really huge breaking change in the API, so I'm afraid we cannot accept it right now according to our migration policies. Please, keep in touch meanwhile. Thank you for understanding! |
…cLock Fixes: spring-projects#3444 * Add `CustomTtlLock`, and `CustomTtlLockRegistry` interfaces * Modify `RedisLockRegistry` to implement the interfaces. * Modify ddl of `INT_LOCK` table, `LockRepository`, `DefaultLockRepository`, and `JdbcLockRegistry` to implement the interfaces. * Fix potential concurrency issue of `unlock` method of `JdbcLock`. * Maintain existing test cases and add new test cases.
Remove `CustomTtlLockRegistry`
| public void delete(String lock) { | ||
| this.defaultTransactionTemplate.executeWithoutResult( | ||
| transactionStatus -> this.template.update(this.deleteQuery, this.region, lock, this.id)); | ||
| public boolean delete(String lock) { |
There was a problem hiding this comment.
This also presents an opportunity to check for template.update() > 1 to throw exception, this will help prevent incorrect constraint on Table.
This is possible only if table is created without constraint INT_LOCK_PK primary key (LOCK_KEY, REGION).
There was a problem hiding this comment.
This is actually outside the scope of the PR. (my bad, I should not include this change).
The method has been maintained in PR #9292.
I think business logic should not address cases of misconfiguration or incorrect settings.
If you'd like to discuss this method further, it would be great to create a new issue.
|
Any new on this? Spring 7 will be out soon right? Will these changes be included 7.0? Spring framework will become GA in Nov 2024 -https://spring.io/blog/2024/10/01/from-spring-framework-6-2-to-7-0. |
|
Thank you both for keeping an eye on this! So, in November we are releasing |
Fixes: #3444
CustomTtlLock, andCustomTtlLockRegistryinterfacesRedisLockRegistryto implement the interfaces.INT_LOCKtable,LockRepository,DefaultLockRepository, andJdbcLockRegistryto implement the interfaces.unlockmethod ofJdbcLock.