Skip to content

RedisLockRegistry UNLINK command (optimization) #3434

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

Closed
FranPregernik opened this issue Dec 1, 2020 · 0 comments · Fixed by #3436
Closed

RedisLockRegistry UNLINK command (optimization) #3434

FranPregernik opened this issue Dec 1, 2020 · 0 comments · Fixed by #3436
Assignees
Milestone

Comments

@FranPregernik
Copy link

In what version(s) of Spring Integration are you seeing this issue?

5.4.1

Describe the bug

The RedisLockRegistry is creating RedisLock instances that test the Redis backend if it supports the UNLINK command.
If it does not, a local variable unlinkAvailable is set to false and the error is logged as a warning.
This causes the RedisLock instance to use the old DELETE command.

There are two issues with this.

First one is that each new RedisLock instance will do the above logic over and over again (for every new instance once). Since Lock instances are mostly short lived that is a lot of UNLINK command testing towards the same Redis backend. We can assume the Redis version does not change during the runtime of the application.

The other, smaller, issue is that the log warning message in its current form can't be silenced/minimized and always outputs an Exception stack trace which adds a great deal of noise to the log.

To Reproduce

Point the RedisLockRegistry to an older version of Redis that does not support the UNLINK command.

Expected behavior

This detection of UNLINK support should be moved to the RedisLockRegistry and the unlinkAvailable command should be passed as a constructor param to RedisLock (base idea by @artembilan #3041 (comment))

The log warning message should be logged without the stack trace and if the stack trace is important it should be guarded under a log.isDebugEnabled or similar.

Sample

Sorry... might be too basic of an example to warrant a sample. It would have to be some kind of docker compose example to include an older version of Redis... or TestContainers...

@FranPregernik FranPregernik added status: waiting-for-triage The issue need to be evaluated and its future decided type: bug labels Dec 1, 2020
@artembilan artembilan added this to the 5.4.2 milestone Dec 1, 2020
@artembilan artembilan added backport 5.2.x in: redis and removed status: waiting-for-triage The issue need to be evaluated and its future decided labels Dec 1, 2020
@artembilan artembilan self-assigned this Dec 1, 2020
artembilan added a commit to artembilan/spring-integration that referenced this issue Dec 1, 2020
Fixes spring-projects#3434

Each `RedisLock` has their own `unlinkAvailable` property
therefore we try to `unlink` on every unlock call and also log a WARN
about unavailability of this command in Redis, plus exception

* Move `unlinkAvailable` property to the `RedisLockRegistry` level
to have a check only once on a first `unlock()` call
* Move the whole exception logging onto the DEBUG level, leaving the WARN
only with the `ex.getMessage()`
* Optimize logging the same way in the `RedisMessageStore`

Cherry-pick to `5.3.x & 5.2.x`
garyrussell pushed a commit that referenced this issue Dec 1, 2020
Fixes #3434

Each `RedisLock` has their own `unlinkAvailable` property
therefore we try to `unlink` on every unlock call and also log a WARN
about unavailability of this command in Redis, plus exception

* Move `unlinkAvailable` property to the `RedisLockRegistry` level
to have a check only once on a first `unlock()` call
* Move the whole exception logging onto the DEBUG level, leaving the WARN
only with the `ex.getMessage()`
* Optimize logging the same way in the `RedisMessageStore`

Cherry-pick to `5.3.x & 5.2.x`
garyrussell pushed a commit that referenced this issue Dec 1, 2020
Fixes #3434

Each `RedisLock` has their own `unlinkAvailable` property
therefore we try to `unlink` on every unlock call and also log a WARN
about unavailability of this command in Redis, plus exception

* Move `unlinkAvailable` property to the `RedisLockRegistry` level
to have a check only once on a first `unlock()` call
* Move the whole exception logging onto the DEBUG level, leaving the WARN
only with the `ex.getMessage()`
* Optimize logging the same way in the `RedisMessageStore`

Cherry-pick to `5.3.x & 5.2.x`
garyrussell pushed a commit that referenced this issue Dec 1, 2020
Fixes #3434

Each `RedisLock` has their own `unlinkAvailable` property
therefore we try to `unlink` on every unlock call and also log a WARN
about unavailability of this command in Redis, plus exception

* Move `unlinkAvailable` property to the `RedisLockRegistry` level
to have a check only once on a first `unlock()` call
* Move the whole exception logging onto the DEBUG level, leaving the WARN
only with the `ex.getMessage()`
* Optimize logging the same way in the `RedisMessageStore`

Cherry-pick to `5.3.x & 5.2.x`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants