Zoltan Borok-Nagy 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) Thanks! 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 UnregisterSessionTimeo Right. OK, I changed the visibility of the timeout methods. 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. SetTimeout() is only called at places that modified session_timeout and query options already. 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 lo It is not needed to hold the mutex during the notify call in RegisterSessionTimeout either, so I fixed it as well. It is valid to call notify on a condition variable without holding the associated mutex. And it is not just valid, but more efficient also, because otherwise the notified thread would block on the mutex immediately. More on that: https://stackoverflow.com/questions/17101922/do-i-have-to-acquire-lock-before-calling-condition-variable-notify-one/ 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 g Yeah, it's definitely regular or advanced. Maybe regular, but I don't have a strong opinion about it. 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 e Done, I chose the second option. 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 runti I reduced the timeouts, also in JdbcTest.java. 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 mi Tried those builds, there were no problems on my PC. -- 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: Mon, 27 Nov 2017 15:03:51 +0000 Gerrit-HasComments: Yes
