You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/12/08 13:06:19 UTC
[GitHub] [druid] imply-cheddar opened a new pull request #12041: Allow for appending tasks to co-exist with each other.
imply-cheddar opened a new pull request #12041:
URL: https://github.com/apache/druid/pull/12041
### Description
Add a config parameter for appending tasks to allow them to
use a SHARED lock. This will allow multiple appending tasks
to add segments to the same datasource at the same time.
This config should actually be the default, but it is added
as a config to enable a smooth transition/validation in
production settings before forcing it as the default
behavior going forward.
This change leverages the TaskLockType.SHARED that existed
previously, this used to carry the semantics of a READ lock,
which was "escalated" when the task wanted to actually
persist the segment. As of many moons before this diff, the
SHARED lock had stopped being used but was still piped into
the code. It turns out that with a few tweaks, it can be
adjusted to be a shared lock for append tasks to allow them
all to write to the same datasource, so that is what this does.
##### Key changed/added classes in this PR
* New task context on append tasks: `useSharedLock`. Defaults to false. Exists to validate functionality in production environments before shared locks are made the default and only type of lock for appending tasks to use.
This PR has:
- [x] been self-reviewed.
- [ ] added documentation for new or modified features or behaviors.
- [ ] 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](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] 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](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #12041: Allow for appending tasks to co-exist with each other.
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #12041:
URL: https://github.com/apache/druid/pull/12041#discussion_r765278116
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java
##########
@@ -1136,6 +1136,9 @@ boolean reusableFor(LockRequest request)
case SHARED:
// All shared lock is not reusable. Instead, a new lock posse is created for each lock request.
Review comment:
Can you fix this comment? It seems no longer true.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java
##########
@@ -581,7 +581,7 @@ private boolean isTaskLocksValid(Task task, List<Interval> intervals)
final List<TaskLockPosse> lockPosses = getOnlyTaskLockPosseContainingInterval(task, interval);
// Tasks cannot enter the critical section with a shared lock
Review comment:
Can you remove this comment?
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java
##########
@@ -581,7 +581,7 @@ private boolean isTaskLocksValid(Task task, List<Interval> intervals)
final List<TaskLockPosse> lockPosses = getOnlyTaskLockPosseContainingInterval(task, interval);
// Tasks cannot enter the critical section with a shared lock
return lockPosses.stream().map(TaskLockPosse::getTaskLock).allMatch(
Review comment:
> [ERROR] indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskLockbox.java:583 -- Stream.allMatch(x -> !(...)) can be replaced with noneMatch(...)
Intellij inspection failed at this line.
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractBatchIndexTask.java
##########
@@ -286,10 +286,11 @@ public boolean determineLockGranularityAndTryLock(TaskActionClient client, List<
Tasks.FORCE_TIME_CHUNK_LOCK_KEY,
Tasks.DEFAULT_FORCE_TIME_CHUNK_LOCK
);
+ final boolean useSharedLock = getContextValue(Tasks.USE_SHARED_LOCK, false);
Review comment:
Should we also check if `appendingToExisting` is set to true?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org