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 2020/09/23 20:14:10 UTC
[GitHub] [druid] clintropolis opened a new pull request #10426: more timeout handling in JsonParserIterator
clintropolis opened a new pull request #10426:
URL: https://github.com/apache/druid/pull/10426
### Description
This PR adds more robust timeout checking in `JsonParserIterator`, as well as more consistent exceptions thrown from the class (everything should now be wrapped in a `QueryInterruptedException`). Not entirely sure where to begin to try to write a test for this, so.. i haven't for now. However, none of the codepaths have really changed, everywhere that was previously throwing some subclass of RuntimeException will continue to do so, there is just a bit more variation on which exceptions will be thrown.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] 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.
- [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.
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] a2l007 commented on pull request #10426: more timeout handling in JsonParserIterator
Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10426:
URL: https://github.com/apache/druid/pull/10426#issuecomment-697969996
This might not be within the scope of this PR, but do you think it would be useful to separate out timeout exceptions from `QueryInterruptedException`? We could have something like a `QueryTimeoutException` being thrown in case of timeouts and `QueryResource` can handle this separately by returning a 504 status instead of 500. I feel returning the 504 error code would be more useful for cluster operators to differentiate between query timeouts vs other types of failures.
----------------------------------------------------------------
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.
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 merged pull request #10426: more timeout handling in JsonParserIterator
Posted by GitBox <gi...@apache.org>.
clintropolis merged pull request #10426:
URL: https://github.com/apache/druid/pull/10426
----------------------------------------------------------------
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.
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 pull request #10426: more timeout handling in JsonParserIterator
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10426:
URL: https://github.com/apache/druid/pull/10426#issuecomment-697999289
>This might not be within the scope of this PR, but do you think it would be useful to separate out timeout exceptions from QueryInterruptedException? We could have something like a QueryTimeoutException being thrown in case of timeouts and QueryResource can handle this separately by returning a 504 status instead of 500. I feel returning the 504 error code would be more useful for cluster operators to differentiate between query timeouts vs other types of failures.
Yeah, out of scope of what I was trying to do here, but it is probably worth discussing if `QueryInterruptedException` should be decomposed into several different exceptions - I don't know the answer but it does come up from time to time. It does a reasonable job of encoding the exception information, but it does feel a bit rough. We could potentially even look for `QueryInterruptedException.QUERY_TIMEOUT` as the error and vary the response status code that way, without splitting out the exceptions, but it might be a bit cleaner if we did.
----------------------------------------------------------------
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.
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 pull request #10426: more timeout handling in JsonParserIterator
Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10426:
URL: https://github.com/apache/druid/pull/10426#issuecomment-700255958
Thanks for the review @a2l007 :+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.
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