Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12579 )
Change subject: IMPALA-7800: Reject new connections after --fe_service_threads ...................................................................... Patch Set 10: (9 comments) LGTM. Mostly nits. http://gerrit.cloudera.org:8080/#/c/12579/10//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12579/10//COMMIT_MSG@7 PS10, Line 7: Reject new connections after --fe_service_threads Does this need to be updated too ? http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.h File be/src/rpc/TAcceptQueueServer.h: http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.h@65 PS10, Line 65: timeout nit: timeout_ms http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.h@116 PS10, Line 116: /// Amount of time after which a connection request will be timed out. in milliseconds http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.cpp File be/src/rpc/TAcceptQueueServer.cpp: http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/TAcceptQueueServer.cpp@263 PS10, Line 263: if (queue_timeout_ms_ != 0) : entry->expiration_time_ = MonotonicMillis() + queue_timeout_ms_; May need to guard against negative value although it's user's fault in that case. if (queue_timeout_ms > 0) { .... } http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h File be/src/rpc/thrift-server.h: http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h@149 PS10, Line 149: int64_t queue_timeout_ms = 0); May make sense to document the meaning of this argument too. nit: this line seems to fit into the previous line too. http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h@197 PS10, Line 197: /// Amount of time an accepted client connection will be kept in the accept : /// queue before it is timed out. Used in TAcceptQueueServer. May want to document that if it's 0, it means there is no timeout (similar to max_concurrent_connections_ above). http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/rpc/thrift-server.h@277 PS10, Line 277: } nit: missing blank line after this function. http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/12579/10/be/src/service/impala-server.cc@239 PS10, Line 239: accepted_cnxn_timeout Should this have a more specific name such as "accepted_client_cnxn_timeout" as we use Thrift server for various internal services too ? http://gerrit.cloudera.org:8080/#/c/12579/10/tests/custom_cluster/test_frontend_connection_limit.py File tests/custom_cluster/test_frontend_connection_limit.py: http://gerrit.cloudera.org:8080/#/c/12579/10/tests/custom_cluster/test_frontend_connection_limit.py@90 PS10, Line 90: except Exception as e: How do we catch cases in which the test passes without raising the exception ? -- To view, visit http://gerrit.cloudera.org:8080/12579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idb345c1d84cc2f691f54ded467f253e758f87e64 Gerrit-Change-Number: 12579 Gerrit-PatchSet: 10 Gerrit-Owner: Zoram Thanga <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Zoram Thanga <[email protected]> Gerrit-Comment-Date: Tue, 26 Mar 2019 22:05:22 +0000 Gerrit-HasComments: Yes
