Gabor Kaszab has posted comments on this change. ( http://gerrit.cloudera.org:8080/8490 )
Change subject: IMPALA-2248: Make idle_session_timeout a query option ...................................................................... Patch Set 9: (7 comments) http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.h@441 PS9, Line 441: friend class ClientRequestState; This is required so that ClientRequestState can call UnregisterSessionTimeout() and RegisterSessionTimeout(), right? Would it be possible to make Register- and UnregisterTimeout() public and get rid of the friend class? http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.cc@1240 PS9, Line 1240: std::to_string(this->session_timeout), I have no knowledge about how threading works around this part of the code. Do you know if a race-condition is possible around this->session_timeout? http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/impala-server.cc@1816 PS9, Line 1816: session_timeout_cv_.NotifyOne(); In RegisterSessionTimeout() the notify_one call is also protected by the lock. For my benefit, could you explain me why it was not required here? http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/query-options.h File be/src/service/query-options.h: http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/query-options.h@99 PS9, Line 99: QUERY_OPT_FN(idle_session_timeout, IDLE_SESSION_TIMEOUT)\ For the sake of IMPALA-2181, could you find out which of the 4 new option group this one should belong to? (regular, advanced, development, deprecated). Guess some of the regular of advanced groups, right? http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8490/9/be/src/service/query-options.cc@578 PS9, Line 578: "Only positive numbers are allowed.", value)); I think the '4 spaces for indentation' rule applies here as well. I would either add 2 more spaces or indent the beginning of the string to match the one above (if that didn't make the line too long). http://gerrit.cloudera.org:8080/#/c/8490/9/tests/custom_cluster/test_session_expiration.py File tests/custom_cluster/test_session_expiration.py: http://gerrit.cloudera.org:8080/#/c/8490/9/tests/custom_cluster/test_session_expiration.py@62 PS9, Line 62: sleep(3) Could you please re-work these sleep times a little bit to reduce the runtimes of these tests? These 2 tests spend 14 secs just on sleep. http://gerrit.cloudera.org:8080/#/c/8490/9/tests/custom_cluster/test_session_expiration.py@70 PS9, Line 70: sleep(8) I learnt this week that running the test suite on RELEASE and ASAN build might give you tricky results. Once re-worked these numbers, could you please verify that the tests pass also on those build? (For me a query that took like 0,5 sec to start fetching results apparently succeeded in a DEBUG build but on RELEASE and ASAN failed even if modified the sleep from 0,5 to 1.5 sec) -- To view, visit http://gerrit.cloudera.org:8080/8490 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e Gerrit-Change-Number: 8490 Gerrit-PatchSet: 9 Gerrit-Owner: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Reviewer: Attila Jeges <atti...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Gabor Kaszab <gaborkas...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Thu, 23 Nov 2017 01:51:23 +0000 Gerrit-HasComments: Yes
