-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
GH-3444: Add Custom TTL support for RedisLock, and JdbcLock #9053
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RedisLock
throw IllegalStateException
when it face this scenario.
Should we also modify the implementation of RedisLock
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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`
Fixes: #3444
CustomTtlLock
, andCustomTtlLockRegistry
interfacesRedisLockRegistry
to implement the interfaces.INT_LOCK
table,LockRepository
,DefaultLockRepository
, andJdbcLockRegistry
to implement the interfaces.unlock
method ofJdbcLock
.