Zoram Thanga 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) Thanks Michael. Uploading PS 11 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 ? Done 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 Done 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 Done 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 Done 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. Done 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 Done 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. Done 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 Changed. 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 exceptio The test will be marked as failed in that case (XFAIL). Added an else block with client.close() to not make test verify step wait. -- 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:58:23 +0000 Gerrit-HasComments: Yes
