You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2025/07/01 04:01:41 UTC
[PR] Do not retry task actions when a lock is revoked (druid)
kfaraz opened a new pull request, #18182:
URL: https://github.com/apache/druid/pull/18182
### Description
When a task lock has been revoked, the corresponding task action should not be retried since the task
revoking the lock is likely to be still in progress. It is better to just fail the task with the revoked locks since
new locks will need to be acquired anyway.
### Changes
- Throw a DruidException of forbidden category if task locks are revoked. The `ServiceClient` does not retry these errors.
- Reduce the default number of retries in the remote task action client to 20 (about 10 minutes worth of retries).
---
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] 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](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
Re: [PR] Do not retry task actions when a lock is revoked (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18182:
URL: https://github.com/apache/druid/pull/18182#discussion_r2176455789
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TaskLocks.java:
##########
@@ -57,7 +58,7 @@ static void checkLockCoversSegments(
)
{
if (!isLockCoversSegments(task, taskLockbox, segments)) {
- throw new ISE(
+ throw Forbidden.exception(
Review Comment:
Updated.
--
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
Re: [PR] Do not retry task actions when a lock is revoked (druid)
Posted by "imply-cheddar (via GitHub)" <gi...@apache.org>.
imply-cheddar commented on code in PR #18182:
URL: https://github.com/apache/druid/pull/18182#discussion_r2176376633
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/actions/SegmentTransactionalInsertActionTest.java:
##########
@@ -217,10 +218,11 @@ public void test_fail_badVersion() throws Exception
actionTestKit.getTaskLockbox().add(task);
acquireTimeChunkLock(TaskLockType.EXCLUSIVE, task, INTERVAL, 5000);
- IllegalStateException exception = Assert.assertThrows(
- IllegalStateException.class,
+ DruidException exception = Assert.assertThrows(
+ DruidException.class,
() -> action.perform(task, actionTestKit.getTaskActionToolbox())
);
+ Assert.assertEquals(DruidException.Category.FORBIDDEN, exception.getCategory());
Review Comment:
Why not use the `DruidExceptionMatcher`?
##########
docs/configuration/index.md:
##########
@@ -1475,7 +1475,7 @@ For most types of tasks, `SegmentWriteOutMediumFactory` can be configured per-ta
|`druid.indexer.task.storeEmptyColumns`|Boolean value for whether or not to store empty columns during ingestion. When set to true, Druid stores every column specified in the [`dimensionsSpec`](../ingestion/ingestion-spec.md#dimensionsspec). <br/><br/>If you set `storeEmptyColumns` to false, Druid SQL queries referencing empty columns will fail. If you intend to leave `storeEmptyColumns` disabled, you should either ingest placeholder data for empty columns or else not query on empty columns.<br/><br/>You can overwrite this configuration by setting `storeEmptyColumns` in the [task context](../ingestion/tasks.md#context-parameters).|true|
|`druid.peon.taskActionClient.retry.minWait`|The minimum retry time to communicate with Overlord.|`PT5S`|
|`druid.peon.taskActionClient.retry.maxWait`|The maximum retry time to communicate with Overlord.|`PT1M`|
-|`druid.peon.taskActionClient.retry.maxRetryCount`|The maximum number of retries to communicate with Overlord.|60|
+|`druid.peon.taskActionClient.retry.maxRetryCount`|The maximum number of retries to communicate with Overlord.|20 (about 10 minutes of retrying)|
Review Comment:
20 is 17.5 minutes of retries, not 10 minutes.
--
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
Re: [PR] Do not retry task actions when a lock is revoked (druid)
Posted by "gianm (via GitHub)" <gi...@apache.org>.
gianm commented on code in PR #18182:
URL: https://github.com/apache/druid/pull/18182#discussion_r2176427724
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TaskLocks.java:
##########
@@ -57,7 +58,7 @@ static void checkLockCoversSegments(
)
{
if (!isLockCoversSegments(task, taskLockbox, segments)) {
- throw new ISE(
+ throw Forbidden.exception(
Review Comment:
This is kind of hacky, since "Forbidden" leads to `403 Forbidden`, which is meant for situations where an action is unauthorized based on security configuration. IMO it would be better to return a different code, like `400 Bad Request` or `409 Conflict`. Those are both considered nonretryable by the standard retry policy.
--
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
Re: [PR] Do not retry task actions when a lock is revoked (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #18182:
URL: https://github.com/apache/druid/pull/18182#issuecomment-3023666214
Thanks a lot for the reviews, @gianm , @imply-cheddar !
--
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
Re: [PR] Do not retry task actions when a lock is revoked (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz merged PR #18182:
URL: https://github.com/apache/druid/pull/18182
--
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
Re: [PR] Do not retry task actions when a lock is revoked (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18182:
URL: https://github.com/apache/druid/pull/18182#discussion_r2176456532
##########
docs/configuration/index.md:
##########
@@ -1475,7 +1475,7 @@ For most types of tasks, `SegmentWriteOutMediumFactory` can be configured per-ta
|`druid.indexer.task.storeEmptyColumns`|Boolean value for whether or not to store empty columns during ingestion. When set to true, Druid stores every column specified in the [`dimensionsSpec`](../ingestion/ingestion-spec.md#dimensionsspec). <br/><br/>If you set `storeEmptyColumns` to false, Druid SQL queries referencing empty columns will fail. If you intend to leave `storeEmptyColumns` disabled, you should either ingest placeholder data for empty columns or else not query on empty columns.<br/><br/>You can overwrite this configuration by setting `storeEmptyColumns` in the [task context](../ingestion/tasks.md#context-parameters).|true|
|`druid.peon.taskActionClient.retry.minWait`|The minimum retry time to communicate with Overlord.|`PT5S`|
|`druid.peon.taskActionClient.retry.maxWait`|The maximum retry time to communicate with Overlord.|`PT1M`|
-|`druid.peon.taskActionClient.retry.maxRetryCount`|The maximum number of retries to communicate with Overlord.|60|
+|`druid.peon.taskActionClient.retry.maxRetryCount`|The maximum number of retries to communicate with Overlord.|20 (about 10 minutes of retrying)|
Review Comment:
Thanks for catching this! I was testing out with the default values in `StandardRetryPolicy.unlimited()`.
Fixed this up and added a test for future reference.
##########
indexing-service/src/test/java/org/apache/druid/indexing/common/actions/SegmentTransactionalInsertActionTest.java:
##########
@@ -217,10 +218,11 @@ public void test_fail_badVersion() throws Exception
actionTestKit.getTaskLockbox().add(task);
acquireTimeChunkLock(TaskLockType.EXCLUSIVE, task, INTERVAL, 5000);
- IllegalStateException exception = Assert.assertThrows(
- IllegalStateException.class,
+ DruidException exception = Assert.assertThrows(
+ DruidException.class,
() -> action.perform(task, actionTestKit.getTaskActionToolbox())
);
+ Assert.assertEquals(DruidException.Category.FORBIDDEN, exception.getCategory());
Review Comment:
Updated.
--
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
Re: [PR] Do not retry task actions when a lock is revoked (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #18182:
URL: https://github.com/apache/druid/pull/18182#discussion_r2176444707
##########
indexing-service/src/main/java/org/apache/druid/indexing/common/actions/TaskLocks.java:
##########
@@ -57,7 +58,7 @@ static void checkLockCoversSegments(
)
{
if (!isLockCoversSegments(task, taskLockbox, segments)) {
- throw new ISE(
+ throw Forbidden.exception(
Review Comment:
`403` made some sense to me in that this task is forbidden or not authorized to act on the interval in question since it doesn't hold the required locks.
But I guess it does become ambiguous with a real forbidden error where the __user__ making the request is not authorized to perform some action.
--
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