-
Notifications
You must be signed in to change notification settings - Fork 372
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[OPIK-457] Fix Redis lock keys leak #730
Conversation
a13cc2f
to
bffc42a
Compare
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.
Great work!
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.
Just two minor comments, the rest LGTM. Thanks for putting this together!
__ -> log.debug("Lock {} released successfully", locked), | ||
__ -> log.warn("Lock {} already released", locked)); |
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.
Minor: wrap escaped values in the logs between single quotes.
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.
Adding both in a next review
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertFalse; |
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.
Minor: please clean up all these JUnit assertions and move to assertJ.
Details
Radisson doesn't delete a lock key from Redis after releasing it by default. To tackle this issue, we are adding a new configuration,
ttlInSeconds
. This configuration will be used to set a ttl time for the lock every time a thread acquires a lock.Issues
OPIK-457
Testing