Michael Ho 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 17: (11 comments) Thanks for updating the patch. LGTM. Please see some minor comments below. http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-hs2-server.cc@340 PS17, Line 340: state->UpdateTimeout(); This line is a bit subtle. The session hasn't yet registered any timeout so calling UnregisterSessionTimeout() doesn't seem safe. However, state->session_timeout was initialized to 0 which becomes a no-op in UnregisterSessionTimeout(). If my understanding above is correct, do you think it makes sense to DCHECK_EQ(0, state->session_timeout); before this line ? An alternate way is to retain the call to RegisterSessionTimeout() and set state->session_timeout explicitly. http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-hs2-server.cc@341 PS17, Line 341: VLOG_QUERY << "OpenSession(): idle_session_timeout=" : << PrettyPrinter::Print(state->session_timeout, TUnit::TIME_S); Is it worth logging this every time a session is opened ? What you did in PS15 seems better. http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1229 PS17, Line 1229: nit: blank space here seems unnecessary. http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1231 PS17, Line 1231: nit: blank space here seems unnecessary. http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1237 PS17, Line 1237: nit: blank space here seems unnecessary. http://gerrit.cloudera.org:8080/#/c/8490/17/be/src/service/impala-server.cc@1768 PS17, Line 1768: session_state->UpdateTimeout(); IMHO, the old way of explicitly calling RegisterSessionTimeout() when a session is started seems clearer for reason mentioned in impala-hs2-server.cc. Feel free to disagree though. http://gerrit.cloudera.org:8080/#/c/8490/17/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/8490/17/common/thrift/ImpalaService.thrift@293 PS17, Line 293: are never expired nit: never expire. http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java File fe/src/test/java/org/apache/impala/service/JdbcTest.java: http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@629 PS17, Line 629: + 500 Just curious on reasoning for + 500 ? http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@629 PS17, Line 629: timeoutTolerance So, the larger the timeout, the larger the tolerance is ? http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@629 PS17, Line 629: sleepPeriod sleepPeriodMs. http://gerrit.cloudera.org:8080/#/c/8490/17/fe/src/test/java/org/apache/impala/service/JdbcTest.java@637 PS17, Line 637: lastExecStatementTime "now" seems to be a more appropriate name esp. if you are comparing it against lastTimeSessionActive. -- 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: 17 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Attila Jeges <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 21 Dec 2017 00:45:30 +0000 Gerrit-HasComments: Yes
