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/10/09 05:47:49 UTC
[GitHub] [druid] jon-wei opened a new pull request #11788: Fix indefinite WAITING batch task when lock is revoked
jon-wei opened a new pull request #11788:
URL: https://github.com/apache/druid/pull/11788
When a batch task is added to the TaskQueue but its lock is revoked (such as when a streaming task gets data for a conflicting time interval), the isReady method currently returns false, and the task will never become ready, leading to the task remaining in a WAITING state forever.
This PR causes a revoked lock to throw an exception, causing the task with the revoked lock to fail. The TimeChunkLockTryAcquireAction has been adjusted to return the LockResult instead of the Lock or `null` so that the task's `isReady` method can check the revocation status.
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.
- [x] 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.
- [x] 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] clintropolis commented on a change in pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r727638115
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -81,7 +81,8 @@ public TaskLock perform(Task task, TaskActionToolbox toolbox)
task,
new TimeChunkLockRequest(type, task, interval, null)
);
- return result.isOk() ? result.getTaskLock() : null;
+
+ return result.getTaskLock();
Review comment:
hmm, this doesn't match `TimeChunkLockAcquireAction` anymore, does it also need updated to match this signature?
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractBatchIndexTask.java
##########
@@ -404,6 +405,9 @@ protected boolean tryTimeChunkLock(TaskActionClient client, List<Interval> inter
if (lock == null) {
return false;
}
+ if (lock.isRevoked()) {
Review comment:
i wonder if there is some sort of analysis we could add to make build complain at us if we are getting a task lock anywhere and not checking if it is revoked... I haven't a very good idea on how to actually do this though, just a bit worried that this could be error prone.
It seems a much bigger refactoring to just make the methods that acquire the locks explode and throw some specialized 'lock revoked' exception, and also to sort of violate the current contract of the 'try acquire' methods, so i'm not really sure that is a good approach either.
--
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] jon-wei commented on pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
jon-wei commented on pull request #11788:
URL: https://github.com/apache/druid/pull/11788#issuecomment-953366166
The coverage checks show 0% on `SeekableStreamIndexTaskRunner`, but these paths are exercised in the Kafka tests (these are in another module).
--
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] jon-wei merged pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
jon-wei merged pull request #11788:
URL: https://github.com/apache/druid/pull/11788
--
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] jon-wei commented on a change in pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r727326421
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -67,21 +66,21 @@ public Interval getInterval()
}
@Override
- public TypeReference<TaskLock> getReturnTypeReference()
+ public TypeReference<LockResult> getReturnTypeReference()
{
- return new TypeReference<TaskLock>()
+ return new TypeReference<LockResult>()
{
};
}
@Override
- public TaskLock perform(Task task, TaskActionToolbox toolbox)
+ public LockResult perform(Task task, TaskActionToolbox toolbox)
Review comment:
I changed the action to return a TaskLock again, and had TaskLockbox return the TaskLock with a revoked result, so that tasks can check the isRevoked status on the TaskLock itself. With middle managers upgraded first, this should avoid any issues with backwards compatibility
--
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 #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r725521139
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -67,21 +66,21 @@ public Interval getInterval()
}
@Override
- public TypeReference<TaskLock> getReturnTypeReference()
+ public TypeReference<LockResult> getReturnTypeReference()
{
- return new TypeReference<TaskLock>()
+ return new TypeReference<LockResult>()
{
};
}
@Override
- public TaskLock perform(Task task, TaskActionToolbox toolbox)
+ public LockResult perform(Task task, TaskActionToolbox toolbox)
Review comment:
Hmm is this an incompatible change which can break rolling upgrades? Because the return type can mismatch in tasks and the overlord during rolling upgrades.
--
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] clintropolis commented on a change in pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r727638115
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -81,7 +81,8 @@ public TaskLock perform(Task task, TaskActionToolbox toolbox)
task,
new TimeChunkLockRequest(type, task, interval, null)
);
- return result.isOk() ? result.getTaskLock() : null;
+
+ return result.getTaskLock();
Review comment:
hmm, this doesn't match `TimeChunkLockAcquireAction` anymore, does it also need updated to match this signature?
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractBatchIndexTask.java
##########
@@ -404,6 +405,9 @@ protected boolean tryTimeChunkLock(TaskActionClient client, List<Interval> inter
if (lock == null) {
return false;
}
+ if (lock.isRevoked()) {
Review comment:
i wonder if there is some sort of analysis we could add to make build complain at us if we are getting a task lock anywhere and not checking if it is revoked... I haven't a very good idea on how to actually do this though, just a bit worried that this could be error prone.
It seems a much bigger refactoring to just make the methods that acquire the locks explode and throw some specialized 'lock revoked' exception, and also to sort of violate the current contract of the 'try acquire' methods, so i'm not really sure that is a good approach either.
--
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] jon-wei commented on a change in pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r727326421
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -67,21 +66,21 @@ public Interval getInterval()
}
@Override
- public TypeReference<TaskLock> getReturnTypeReference()
+ public TypeReference<LockResult> getReturnTypeReference()
{
- return new TypeReference<TaskLock>()
+ return new TypeReference<LockResult>()
{
};
}
@Override
- public TaskLock perform(Task task, TaskActionToolbox toolbox)
+ public LockResult perform(Task task, TaskActionToolbox toolbox)
Review comment:
I changed the action to return a TaskLock again, and had TaskLockbox return the TaskLock with a revoked result, so that tasks can check the isRevoked status on the TaskLock itself. With middle managers upgraded first, this should avoid any issues with backwards compatibility
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -81,7 +81,8 @@ public TaskLock perform(Task task, TaskActionToolbox toolbox)
task,
new TimeChunkLockRequest(type, task, interval, null)
);
- return result.isOk() ? result.getTaskLock() : null;
+
+ return result.getTaskLock();
Review comment:
Good call, I updated `TimeChunkLockAcquireAction` and usages to check for lock revocation
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractBatchIndexTask.java
##########
@@ -404,6 +405,9 @@ protected boolean tryTimeChunkLock(TaskActionClient client, List<Interval> inter
if (lock == null) {
return false;
}
+ if (lock.isRevoked()) {
Review comment:
Hm, I had similar concerns, I think we're good:
for `TaskLockbox.tryLock` called by itself:
- `SegmentAllocateAction` already checks `isRevoked()` and `isOk()` on the `LockResult`
- `SegmentLockTryAcquireAction` usages check `isOk()` on the result
- `TimeChunkLockTryAcquireAction` usages are adjusted in this PR
for `TaskLockbox.lock` calls:
- `SegmentLockAcquireAction` usages check `isOk()` on the result
- `TimeChunkLockAcquireAction` usages are adjusted in this PR
--
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] jon-wei commented on a change in pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r727662536
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/task/AbstractBatchIndexTask.java
##########
@@ -404,6 +405,9 @@ protected boolean tryTimeChunkLock(TaskActionClient client, List<Interval> inter
if (lock == null) {
return false;
}
+ if (lock.isRevoked()) {
Review comment:
Hm, I had similar concerns, I think we're good:
for `TaskLockbox.tryLock` called by itself:
- `SegmentAllocateAction` already checks `isRevoked()` and `isOk()` on the `LockResult`
- `SegmentLockTryAcquireAction` usages check `isOk()` on the result
- `TimeChunkLockTryAcquireAction` usages are adjusted in this PR
for `TaskLockbox.lock` calls:
- `SegmentLockAcquireAction` usages check `isOk()` on the result
- `TimeChunkLockAcquireAction` usages are adjusted in this PR
--
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] jon-wei commented on a change in pull request #11788: Fix indefinite WAITING batch task when lock is revoked
Posted by GitBox <gi...@apache.org>.
jon-wei commented on a change in pull request #11788:
URL: https://github.com/apache/druid/pull/11788#discussion_r727660596
##########
File path: indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TimeChunkLockTryAcquireAction.java
##########
@@ -81,7 +81,8 @@ public TaskLock perform(Task task, TaskActionToolbox toolbox)
task,
new TimeChunkLockRequest(type, task, interval, null)
);
- return result.isOk() ? result.getTaskLock() : null;
+
+ return result.getTaskLock();
Review comment:
Good call, I updated `TimeChunkLockAcquireAction` and usages to check for lock revocation
--
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