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 13: (4 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 ? Actually, it seems that the current code separates the setting of the the session's idle timeout (i.e. SetTimeout) from the actual registration / unregistration with session_timeout_thread_. Is there a reason for this separation ? In other words, can we have SetTimeout() as the only exposed interface for changing idle session timeout value ? It will be responsible for determining the new timeout value based on the requested timeout value and FLAGS_idle_session_timeout and call UnregisterSessionTimeout() and RegisterSessionTimeout() if the timeout value changes. 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 requested_timeout is 0. 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 ? 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: if (FLAGS_idle_session_timeout != 0) { if (request_timeout == 0) { return ... } if (request_timeout > FLAGS_idle_session_timeout) { return .... } } -- 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 <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: Tue, 12 Dec 2017 20:17:01 +0000 Gerrit-HasComments: Yes
