[GitHub] [ignite] samaitra commented on issue #6490: IGNITE-7285 Add default query timeout
samaitra commented on issue #6490: IGNITE-7285 Add default query timeout URL: https://github.com/apache/ignite/pull/6490#issuecomment-550754365 > > @pavlukhin Thank you for reviewing and sharing feedback. > > If you note the tests in IgniteCacheDistributedQueryStopOnCancelOrTimeoutSelfTest also, it is similar to Default Query timeout tests. I suppose the reason being we are throwing QueryCancelledException for either manual cancel or query being cancelled due to timeout. I agree throwing a separate exception for timeout vs manual cancel will be better approach to handle different scenarios and help with logging and exception handling. > > I am also thinking if that change can be taken up as separate issue and we can close on default query handling change in this PR. > > @samaitra I can imagine checking exception message (to check that it is timeout) and approximate cancellation time (to check that it was _default_ timeout). What do you think of it? @pavlukhin I looked into the QueryCancelledException class and it has a generic message as "The query was cancelled while executing.". Also my understanding is it is used commonly for both manually cancelled exception cases and also for timeout exception cases. We can consider creating a separate exception when query is cancelled for timeout vs manually cancelled but I think that change can be taken up in separate jira issue and PR considering the scope. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] samaitra commented on issue #6490: IGNITE-7285 Add default query timeout
samaitra commented on issue #6490: IGNITE-7285 Add default query timeout URL: https://github.com/apache/ignite/pull/6490#issuecomment-546778525 > > > Additionally to comments for code lines there are some questions regarding testing approach. Actually, I do not see how different are expectations from test with and without configured default timeout. Basically, it is good to test following: > > > > > > 1. Only default timeout specified -- query is cancelled after it, it is clear that it was cancelled by timeout. > > > 2. Explicit timeout overrides default timeout. > > > > > > > > 1. My understanding is in the method testQueryCancel when we are passing the 3rd argument timeout as false then only default timeout is specified. > > 2. When we are passing the 3rd argument timeout as true then explicit timeout overrides default timeout. > > @samaitra I copied `IgniteCacheLocalQueryDefaultTimeoutSelfTest` and executed it on master. And tests passed, so I suppose tests do not distinguish cancellation after _default timeout_ vs other possible ways. @pavlukhin Thank you for reviewing and sharing feedback. If you note the tests in IgniteCacheDistributedQueryStopOnCancelOrTimeoutSelfTest also, it is similar to Default Query timeout tests. I suppose the reason being we are throwing QueryCancelledException for either manual cancel or query being cancelled due to timeout. I agree throwing a separate exception for timeout vs manual cancel will be better approach to handle different scenarios and help with logging and exception handling. I am also thinking if that change can be taken up as separate issue and we can close on default query handling change in this PR. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] samaitra commented on issue #6490: IGNITE-7285 Add default query timeout
samaitra commented on issue #6490: IGNITE-7285 Add default query timeout URL: https://github.com/apache/ignite/pull/6490#issuecomment-544208026 > Additionally to comments for code lines there are some questions regarding testing approach. Actually, I do not see how different are expectations from test with and without configured default timeout. Basically, it is good to test following: > > 1. Only default timeout specified -- query is cancelled after it, it is clear that it was cancelled by timeout. > 2. Explicit timeout overrides default timeout. 1. My understanding is in the method testQueryCancel when we are passing the 3rd argument timeout as false then only default timeout is specified. 2. When we are passing the 3rd argument timeout as true then explicit timeout overrides default timeout. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] samaitra commented on issue #6490: IGNITE-7285 Add default query timeout
samaitra commented on issue #6490: IGNITE-7285 Add default query timeout URL: https://github.com/apache/ignite/pull/6490#issuecomment-489287855 @pavlukhin I have updated the PR. Please take a look. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] [ignite] samaitra commented on issue #6490: IGNITE-7285 Add default query timeout
samaitra commented on issue #6490: IGNITE-7285 Add default query timeout URL: https://github.com/apache/ignite/pull/6490#issuecomment-487287705 @AMashenkov thank you for reviewing the changes and sharing feedback, I have incorporated the review comments. Please review and share if any further changes need to be made. 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: us...@infra.apache.org With regards, Apache Git Services
