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 13: (4 comments) Thanks for your comments! http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@342 PS13, Line 342: /// Register timeout value upon opening a new session. This will wake up : /// session_timeout_thread_ to update its poll period. : void RegisterSessionTimeout(int32_t timeout); : : /// Unregister timeout value. This will wake up session_timeout_thread_ : /// to update its poll period. : void UnregisterSessionTimeout(int32_t timeout); > Should these functions be private ? I like this idea. This also means that SessionState needs a pointer to the ImpalaServer. And SessionState needs to be a friend of ImpalaServer to access those private methods. See my new patch set how it looks. http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@437 PS13, Line 437: FLAGS_idle_session_timeout is used > Also, it also seems to use FLAGS_idle_session_timeout as the value if reque Yes, I rephrased this comment. http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/impala-server.h@438 PS13, Line 438: This method also sets the query option 'idle_session_timeout' > Is this still true ? No, I removed this part. http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/query-options.cc File be/src/service/query-options.cc: http://gerrit.cloudera.org:8080/#/c/8490/13/be/src/service/query-options.cc@582 PS13, Line 582: else if (requested_timeout == 0 && FLAGS_idle_session_timeout != 0) { : return Status( : Substitute("Session timeout cannot be unlimited, " : "maximum value: $0 seconds", FLAGS_idle_session_timeout)); : } else if (requested_timeout > FLAGS_idle_session_timeout && : FLAGS_idle_session_timeout != 0) { : return Status(Substitute("Session timeout cannot be set longer than $0 seconds", : FLAGS_idle_session_timeout)); : } > Just a minor suggestion: Yeah, I was thinking about it before. This way we have less conditions, but more nesting. I couldn't really decide which one is more readable, but since you are also suggesting this I re-structured the code. -- 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: 13 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: Wed, 13 Dec 2017 14:55:40 +0000 Gerrit-HasComments: Yes
