Returning correct Response Code HTTP 429 when taskQueue reached maxSize #15409
Conversation
….indexer.queue.maxSize has reached and one more task is submitted.
| @@ -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), " + | |||
There was a problem hiding this 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()
There was a problem hiding this 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.
There was a problem hiding this comment.
Once you get rid of TooManyException class, the PR will also have less changes IMO
There was a problem hiding this comment.
Added all review changes suggested.
- Removed TooManyTasksException
- added DruidException with status code 429
- modified the error msg as well
Tested and attached screenshot.
| @@ -3735,7 +3736,7 @@ public void testGetCurrentTotalStats() | |||
|
|
|||
| @Test | |||
| public void testDoNotKillCompatibleTasks() | |||
| throws InterruptedException, EntryExistsException | |||
| throws InterruptedException, EntryExistsException, DruidException | |||
There was a problem hiding this comment.
I am curious as to why you declared DruidException here. It's a runtime exception.
| catch (DruidException e) { | ||
| return Response.status(e.getResponseCode()) | ||
| .entity(ImmutableMap.of("error", e.getMessage())) | ||
| .build(); |
There was a problem hiding this 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
| @@ -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) { | |||
There was a problem hiding this comment.
lets not catch this exception here as we were not doing it before too.
| @@ -51,7 +52,7 @@ public void setup() | |||
| } | |||
|
|
|||
| @Test | |||
| public void testDefault() throws EntryExistsException | |||
| public void testDefault() throws EntryExistsException, DruidException | |||
There was a problem hiding this comment.
these changes are not needed.
…g old DruidException to OverlordResource
|
Thank you @oo007 for your first contribution. |
|
That was fast. Thank you for all the support @abhishekagarwal87 . Much appreciated :-) |
…ize (apache#15409) 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 apache#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.
Fixes #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 #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
Release note
Returning correct Response Code HTTP 429 when taskQueue reached maxSize
Key changed/added classes in this PR
TooManyTasksExceptionTaskQueueOverlordResourceSeekableStreamSupervisorTaskQueueTestKinesisSupervisorTestTaskLockConfigTestSeekableStreamSupervisorStateTestThis PR has: