-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Ignore append locks for compaction when using concurrent locks #16316
Ignore append locks for compaction when using concurrent locks #16316
Conversation
Boolean.TRUE.equals(lockFilter.getContext().getOrDefault( | ||
Tasks.USE_CONCURRENT_LOCKS, | ||
Tasks.DEFAULT_USE_CONCURRENT_LOCKS)) | ||
|| TaskLockType.REPLACE.name().equals(lockFilter.getContext().getOrDefault( | ||
Tasks.TASK_LOCK_TYPE, | ||
Tasks.DEFAULT_TASK_LOCK_TYPE)); |
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.
Slightly reformatted for better readability since this condition has become a little difficult now.
Boolean.TRUE.equals(lockFilter.getContext().getOrDefault( | |
Tasks.USE_CONCURRENT_LOCKS, | |
Tasks.DEFAULT_USE_CONCURRENT_LOCKS)) | |
|| TaskLockType.REPLACE.name().equals(lockFilter.getContext().getOrDefault( | |
Tasks.TASK_LOCK_TYPE, | |
Tasks.DEFAULT_TASK_LOCK_TYPE)); | |
Boolean.TRUE.equals( | |
lockFilter.getContext().getOrDefault( | |
Tasks.USE_CONCURRENT_LOCKS, | |
Tasks.DEFAULT_USE_CONCURRENT_LOCKS | |
) | |
) | |
|| TaskLockType.REPLACE.name().equals( | |
lockFilter.getContext().getOrDefault( | |
Tasks.TASK_LOCK_TYPE, | |
Tasks.DEFAULT_TASK_LOCK_TYPE | |
) | |
); |
You may also consider breaking up the condition.
final boolean isUsingConcurrentLocks = condition1;
final boolean isUsingReplaceLock = condition2;
final boolean ignoreAppendLocks = isUsingConcurrentLocks || isReplaceLock;
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, I've broken up the condition.
I've also verified that the USE_CONCURRENT_LOCKS flag is being used wherever TASK_LOCK_TYPE is, and also that it supersedes the latter.
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 the fix, @AmatyaAvadhanula !
When using the new concurrent lock REPLACE, compaction should not skip intervals locked by APPEND locks.
There was a check in place for the context value
TASK_LOCK_TYPE
.This PR adds the check for the key
USE_CONCURRENT_LOCKS
and also adds a relevant test.This PR has: