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

Ignore append locks for compaction when using concurrent locks #16316

Merged

Conversation

AmatyaAvadhanula
Copy link
Contributor

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:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

Comment on lines 964 to 969
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));
Copy link
Contributor

@kfaraz kfaraz Apr 22, 2024

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.

Suggested change
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;

Copy link
Contributor Author

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.

Copy link
Contributor

@kfaraz kfaraz left a 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 !

@AmatyaAvadhanula AmatyaAvadhanula merged commit 08b5a8b into apache:master Apr 22, 2024
85 checks passed
@kfaraz kfaraz deleted the fix-lock-filter-for-compaction branch April 23, 2024 03:07
@adarshsanjeev adarshsanjeev added this to the 30.0.0 milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants