You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "oo007 (via GitHub)" <gi...@apache.org> on 2023/11/21 07:18:52 UTC
[PR] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
oo007 opened a new pull request, #15409:
URL: https://github.com/apache/druid/pull/15409
Fixes https://github.com/apache/druid/issues/15380
### Description
Currently when we submit a task to druid and number of currently active tasks has already reached (`druid.indexer.queue.maxSize`) then 500 ISE is thrown as per shown in the screenshot in https://github.com/apache/druid/issues/15380.
This fix will return `HTTP 429 Too Many Requests`(with proper error message) instead of 500 ISE, when we submit a task and queueSize has reached.
Tested and attached the screenshot
<img width="600" alt="Screenshot 2023-11-21 at 12 37 34 PM" src="https://github.com/apache/druid/assets/11345667/ad94736f-d3ef-4e61-bf67-8b00ae9a89a8">
#### Release note
Returning correct Response Code HTTP 429 when taskQueue reached maxSize
##### Key changed/added classes in this PR
* `TooManyTasksException`
* `TaskQueue`
* `OverlordResource`
* `SeekableStreamSupervisor`
* `TaskQueueTest`
* `KinesisSupervisorTest`
* `TaskLockConfigTest`
* `SeekableStreamSupervisorStateTest`
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #15409:
URL: https://github.com/apache/druid/pull/15409#discussion_r1400848780
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/http/OverlordResource.java:
##########
@@ -226,10 +227,11 @@ public Response taskPost(final Task task, @Context final HttpServletRequest req)
taskQueue.add(task);
return Response.ok(ImmutableMap.of("task", task.getId())).build();
}
- catch (DruidException e) {
- return Response.status(e.getResponseCode())
- .entity(ImmutableMap.of("error", e.getMessage()))
- .build();
Review Comment:
You still need to retain this, unfortunately. The other DruidException could be caught here as well. You can remove the other deprecated DruidException in a follow-up clean up PR
##########
indexing-service/src/test/java/org/apache/druid/indexing/overlord/TaskLockConfigTest.java:
##########
@@ -51,7 +52,7 @@ public void setup()
}
@Test
- public void testDefault() throws EntryExistsException
+ public void testDefault() throws EntryExistsException, DruidException
Review Comment:
these changes are not needed.
##########
indexing-service/src/main/java/org/apache/druid/indexing/seekablestream/supervisor/SeekableStreamSupervisor.java:
##########
@@ -3954,6 +3954,10 @@ private void createTasksForGroup(int groupId, int replicas)
stateManager.recordThrowableEvent(e);
log.error("Tried to add task [%s] but it already exists", indexTask.getId());
}
+ catch (DruidException e) {
Review Comment:
lets not catch this exception here as we were not doing it before too.
##########
extensions-core/kinesis-indexing-service/src/test/java/org/apache/druid/indexing/kinesis/supervisor/KinesisSupervisorTest.java:
##########
@@ -3735,7 +3736,7 @@ public void testGetCurrentTotalStats()
@Test
public void testDoNotKillCompatibleTasks()
- throws InterruptedException, EntryExistsException
+ throws InterruptedException, EntryExistsException, DruidException
Review Comment:
I am curious as to why you declared DruidException here. It's a runtime exception.
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "oo007 (via GitHub)" <gi...@apache.org>.
oo007 commented on code in PR #15409:
URL: https://github.com/apache/druid/pull/15409#discussion_r1400546245
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -520,7 +521,10 @@ public boolean add(final Task task) throws EntryExistsException
try {
Preconditions.checkState(active, "Queue is not active!");
Preconditions.checkNotNull(task, "task");
- Preconditions.checkState(tasks.size() < config.getMaxSize(), "Too many tasks (max = %s)", config.getMaxSize());
+ if (tasks.size() >= config.getMaxSize()) {
+ throw new TooManyTasksException(StringUtils.format("Too many tasks (maxQueueSize = %d), " +
Review Comment:
Added all review changes suggested. Tested and attached screenshot.
<img width="600" alt="Screenshot 2023-11-21 at 5 05 10 PM" src="https://github.com/apache/druid/assets/11345667/67de8379-d8dd-4f0c-8c93-eaf07bc106e6">
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "oo007 (via GitHub)" <gi...@apache.org>.
oo007 commented on code in PR #15409:
URL: https://github.com/apache/druid/pull/15409#discussion_r1400546245
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -520,7 +521,10 @@ public boolean add(final Task task) throws EntryExistsException
try {
Preconditions.checkState(active, "Queue is not active!");
Preconditions.checkNotNull(task, "task");
- Preconditions.checkState(tasks.size() < config.getMaxSize(), "Too many tasks (max = %s)", config.getMaxSize());
+ if (tasks.size() >= config.getMaxSize()) {
+ throw new TooManyTasksException(StringUtils.format("Too many tasks (maxQueueSize = %d), " +
Review Comment:
Added all review changes suggested.
1. Removed TooManyTasksException
2. added DruidException with status code 429
3. modified the error msg as well
Tested and attached screenshot.
<img width="600" alt="Screenshot 2023-11-21 at 5 05 10 PM" src="https://github.com/apache/druid/assets/11345667/67de8379-d8dd-4f0c-8c93-eaf07bc106e6">
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 merged PR #15409:
URL: https://github.com/apache/druid/pull/15409
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #15409:
URL: https://github.com/apache/druid/pull/15409#discussion_r1400141808
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -520,7 +521,10 @@ public boolean add(final Task task) throws EntryExistsException
try {
Preconditions.checkState(active, "Queue is not active!");
Preconditions.checkNotNull(task, "task");
- Preconditions.checkState(tasks.size() < config.getMaxSize(), "Too many tasks (max = %s)", config.getMaxSize());
+ if (tasks.size() >= config.getMaxSize()) {
+ throw new TooManyTasksException(StringUtils.format("Too many tasks (maxQueueSize = %d), " +
Review Comment:
+1
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "shiv4289 (via GitHub)" <gi...@apache.org>.
shiv4289 commented on PR #15409:
URL: https://github.com/apache/druid/pull/15409#issuecomment-1822417620
Thank you for all the support @abhishekagarwal87 . Much appreciated :-)
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on code in PR #15409:
URL: https://github.com/apache/druid/pull/15409#discussion_r1400140265
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -520,7 +521,10 @@ public boolean add(final Task task) throws EntryExistsException
try {
Preconditions.checkState(active, "Queue is not active!");
Preconditions.checkNotNull(task, "task");
- Preconditions.checkState(tasks.size() < config.getMaxSize(), "Too many tasks (max = %s)", config.getMaxSize());
+ if (tasks.size() >= config.getMaxSize()) {
+ throw new TooManyTasksException(StringUtils.format("Too many tasks (maxQueueSize = %d), " +
Review Comment:
The "dropping" is misleading. We are not dropping anything, just rejecting the extra task. So lets just say that and please add to "retry later or increase the limit" in the error message.
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -520,7 +521,10 @@ public boolean add(final Task task) throws EntryExistsException
try {
Preconditions.checkState(active, "Queue is not active!");
Preconditions.checkNotNull(task, "task");
- Preconditions.checkState(tasks.size() < config.getMaxSize(), "Too many tasks (max = %s)", config.getMaxSize());
+ if (tasks.size() >= config.getMaxSize()) {
+ throw new TooManyTasksException(StringUtils.format("Too many tasks (maxQueueSize = %d), " +
Review Comment:
Instead of TooManyTasksException, you can create a failure type similar to Forbidden. This failure type can be called TooManyRequests. The failure will have an error code set to 429.
In here, you can create DruidException.fromFailure(TooManyRequests).forPersona(Operator).message(<the message>)
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -520,7 +521,10 @@ public boolean add(final Task task) throws EntryExistsException
try {
Preconditions.checkState(active, "Queue is not active!");
Preconditions.checkNotNull(task, "task");
- Preconditions.checkState(tasks.size() < config.getMaxSize(), "Too many tasks (max = %s)", config.getMaxSize());
+ if (tasks.size() >= config.getMaxSize()) {
+ throw new TooManyTasksException(StringUtils.format("Too many tasks (maxQueueSize = %d), " +
Review Comment:
Once you get rid of TooManyException class, the PR will also have less changes IMO
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "abhishekagarwal87 (via GitHub)" <gi...@apache.org>.
abhishekagarwal87 commented on PR #15409:
URL: https://github.com/apache/druid/pull/15409#issuecomment-1822353276
Thank you @oo007 for your first contribution.
--
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] Returning correct Response Code HTTP 429 when taskQueue reached maxSize (druid)
Posted by "oo007 (via GitHub)" <gi...@apache.org>.
oo007 commented on code in PR #15409:
URL: https://github.com/apache/druid/pull/15409#discussion_r1400546245
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/TaskQueue.java:
##########
@@ -520,7 +521,10 @@ public boolean add(final Task task) throws EntryExistsException
try {
Preconditions.checkState(active, "Queue is not active!");
Preconditions.checkNotNull(task, "task");
- Preconditions.checkState(tasks.size() < config.getMaxSize(), "Too many tasks (max = %s)", config.getMaxSize());
+ if (tasks.size() >= config.getMaxSize()) {
+ throw new TooManyTasksException(StringUtils.format("Too many tasks (maxQueueSize = %d), " +
Review Comment:
Added all review changes suggested.
1. Removed TooManyTasksException
2. added DruidException with status code 429
Tested and attached screenshot.
<img width="600" alt="Screenshot 2023-11-21 at 5 05 10 PM" src="https://github.com/apache/druid/assets/11345667/67de8379-d8dd-4f0c-8c93-eaf07bc106e6">
--
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