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

Allow specifying expireAfter on individual Lock level for RedisLockRegistry #8717

Closed
kotovdv opened this issue Aug 29, 2023 · 1 comment
Closed
Labels
status: duplicate There is already an issue similar to this. The link to it should be present

Comments

@kotovdv
Copy link

kotovdv commented Aug 29, 2023

Expected Behavior

Along with the original signature expose additional method with the following signature

@Override
public Lock obtain(Object lockKey) {
    return this.obtain(lockKey, Duration.ofMillis(expireAfter));
}

public Lock obtain(Object lockKey, Duration expireAfter) {
    //implementation
}

This method will allow user to override the default org.springframework.integration.redis.util.RedisLockRegistry#expireAfter value configured on the class level for each individual lock if needed.

Implementation wise it seems to be rather straight forward, we will need to store expireAfter on the RedisLockRegistry.RedisLock class level and use it from here, and not from the RedisLockRegistry.

Current Behavior

Currently all locks will have expireAfter value equal to the value configured on the class level and there is no way to override it without creating a completely new RedisLockRegistry which creates its own problems (local locks objects will not be shared between them).

Context

We have some scenarios where a lock has to be held for a short period of time (<10-15 seconds) and a bunch of scenarios where a lock can be held for a long period of time (~10-15 minutes). In current implementation we either have to tune expireAfter param against the longest possible duration OR have a separate RedisLockRegistry for long lived scenarios.

Tuning the registry against longest value will potentially harm short lived scenarios (we might lock a resource for too long if we failed to release a lock properly) and having separate RedisLockRegistry for different scenarios also seems like an anti-pattern that exists solely because of the current limitation with expireAfter being a class level field with no option to override it on per lock basis.

@kotovdv kotovdv added status: waiting-for-triage The issue need to be evaluated and its future decided type: enhancement labels Aug 29, 2023
@artembilan
Copy link
Member

I treat this as a duplication of: #3444.

See what we have discussed over there.
I'm so sorry that we didn't have a chance to jump into this during 6.0 timeline.

I'm afraid the proposed refactoring won't make it until 7.0 in a couple years, unless we can come up with separate abstraction like DistributedLockRegistry with all the mentioned details, even if some of them are copy-n-paste from the current one.

@artembilan artembilan closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
@artembilan artembilan added status: duplicate There is already an issue similar to this. The link to it should be present and removed type: enhancement status: waiting-for-triage The issue need to be evaluated and its future decided labels Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: duplicate There is already an issue similar to this. The link to it should be present
Projects
None yet
Development

No branches or pull requests

2 participants