Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/9227 )
Change subject: IMPALA-6482: add EXEC_TIME_LIMIT_S option ...................................................................... Patch Set 9: Code-Review+2 (9 comments) http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.h@1038 PS9, Line 1038: a query query execution http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc@984 PS9, Line 984: has timeout of might be nice to make these more specific: has an idle timeout of http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc@990 PS9, Line 990: has time limit has an execution time limit of http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc@1848 PS9, Line 1848: If the last-observed expiration time for this query is still in the future, not your change, but this seems wrong given that queries_by_timestamp has deadlines for various queries right? http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc@1864 PS9, Line 1864: time execution time? http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc@1865 PS9, Line 1865: time execution time? http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc@1872 PS9, Line 1872: DCHECK(kind == IDLE_TIMEOUT) http://gerrit.cloudera.org:8080/#/c/9227/9/be/src/service/impala-server.cc@1874 PS9, Line 1874: it last_active_ms? http://gerrit.cloudera.org:8080/#/c/9227/9/tests/custom_cluster/test_query_expiration.py File tests/custom_cluster/test_query_expiration.py: http://gerrit.cloudera.org:8080/#/c/9227/9/tests/custom_cluster/test_query_expiration.py@88 PS9, Line 88: self.assert_impalad_log_contains('INFO', "Expiring query due to client inactivity: " : "[0-9a-f]+:[0-9a-f]+, last activity was at: \d\d\d\d-\d\d-\d\d \d\d:\d\d:\d\d") : self.assert_impalad_log_contains('INFO', : "Expiring query [0-9a-f]+:[0-9a-f]+ due to time limit of 1s") why do we do that instead of checking the query status message? which we do below with __expect_expired, no? -- To view, visit http://gerrit.cloudera.org:8080/9227 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id81772ee223ffb64746e241027a5a734a811e1b8 Gerrit-Change-Number: 9227 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 23 Feb 2018 00:06:32 +0000 Gerrit-HasComments: Yes
