Skip to content

Ignore append locks for compaction when using concurrent locks#16316

Merged
AmatyaAvadhanula merged 2 commits intoapache:masterfrom
AmatyaAvadhanula:fix-lock-filter-for-compaction
Apr 22, 2024
Merged

Ignore append locks for compaction when using concurrent locks#16316
AmatyaAvadhanula merged 2 commits intoapache:masterfrom
AmatyaAvadhanula:fix-lock-filter-for-compaction

Conversation

@AmatyaAvadhanula
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@AmatyaAvadhanula AmatyaAvadhanula requested a review from kfaraz April 22, 2024 15:33
Copy link
Copy Markdown
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
@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.

3 participants