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 2022/02/19 21:44:54 UTC
[GitHub] [druid] machine424 opened a new pull request #12271: Stop altering the query timeout in ScanQueryEngine
machine424 opened a new pull request #12271:
URL: https://github.com/apache/druid/pull/12271
Fixes https://github.com/apache/druid/issues/12197.
### Description
See https://github.com/apache/druid/issues/12197#issuecomment-1025143613 for details.
I'll squash the commits later, it's just to let you check that the current code fails on the added test.
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.
- [ ] 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
[GitHub] [druid] machine424 edited a comment on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
machine424 edited a comment on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055447651
I think this prevents shortening the timeout in `responseContext` and thus timing out early.
I've added a check in a unit test to make sure we don't alter the timeout. I could add more tests later, when I have time.
But now I need someone to confirm I'm on the right track, maybe we could ask review from someone who is familiar with this part of the code.
--
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] machine424 edited a comment on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
machine424 edited a comment on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055447651
I think this prevents shortening the timeout in `responseContext` and thus timing out early.
I've added a check in a unit test to make sure we don't alter the timeout. I could add more tests later, when I have time.
But now I need someone to **confirm** I'm on the right track (or **prove** that I'm wrong), maybe we could ask review from someone who is familiar with this part of the code.
--
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] machine424 commented on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
machine424 commented on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1068827646
@jihoonson maybe?
--
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] kfaraz edited a comment on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
kfaraz edited a comment on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055225826
Thanks for the changes, @machine424 .
Not sure why this was put here in the first place.
But please note that even the original code was not altering the query timeout (the `timeoutAt` is a final field). It was only altering the timeout value inside the `ResponseContext` object.
+1 after CI passes.
--
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] kfaraz edited a comment on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
kfaraz edited a comment on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055225826
Thanks for the changes, @machine424 .
Not sure why this was put here in the first place.
But please note that even the original code was not altering the query timeout (because the `timeoutAt` is a final field). It was only altering the timeout value inside the `ResponseContext` object.
+1 after CI passes.
--
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] kfaraz commented on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
kfaraz commented on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055230205
Since the `timeoutAt` is not being altered, I don't think this will fix #12197
--
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] kfaraz commented on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
kfaraz commented on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055385273
Thanks for the clarification, @machine424 .
Sorry, but I am still confused how this would fix #12197 .
If it does indeed fix the root cause as you verified, please add a unit test which asserts the same, i.e. a query which times out correctly after the changes.
--
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] kfaraz commented on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
kfaraz commented on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055225826
Thanks for the changes, @machine424 .
Not sure why this was put here in the first place.
But please note that even the original code was not altering the timeout (because the `timeoutAt` is a final field). It was only altering the timeout value inside the `ResponseContext` object.
+1 after CI passes.
--
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] machine424 commented on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
machine424 commented on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055280863
Hi @kfaraz,
Thanks for your response.
I maybe misspoke, but we retrieve the timeout from the response context here:
https://github.com/apache/druid/blob/3f709db173d779db1466e57a1564a5f557b4b0cf/processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java#L75
we check it here:
https://github.com/apache/druid/blob/3f709db173d779db1466e57a1564a5f557b4b0cf/processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java#L174-L176
And we alter it (in the response context) here:
https://github.com/apache/druid/blob/3f709db173d779db1466e57a1564a5f557b4b0cf/processing/src/main/java/org/apache/druid/query/scan/ScanQueryEngine.java#L188-L192
(Of course the compiler wouldn't have appreciated altering an immutable field ^^)
And I did test the changes, as I said in https://github.com/apache/druid/issues/12197#issuecomment-1025143613:
> ...After commenting out Lines 190 to 194, the query's timeout (As I understand it) seems to be respected. Of course it's always possible to exceed it (while in rowsToList for example if the batchSize is big) but this is another subject which is discussed in https://github.com/apache/druid/issues/1415.
--
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] machine424 commented on pull request #12271: Stop altering the query timeout in ScanQueryEngine
Posted by GitBox <gi...@apache.org>.
machine424 commented on pull request #12271:
URL: https://github.com/apache/druid/pull/12271#issuecomment-1055447651
I thin this prevents shortening the timeout in `responseContext` and thus timing out early.
I've added a check in a unit test to make sure we don't alter the timeout. I could add more tests later, when I have time.
But now I need someone to confirm I'm on the right track, maybe we could ask review from someone who is familiar with this part of the code.
--
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