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 8: (5 comments) Thanks for your comments, Andrew. Please see PS 9. http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.h File be/src/rpc/TAcceptQueueServer.h: http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.h@97 PS8, Line 97: /// Name of the thrift server > Add terminating period (nit). Done http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.h@113 PS8, Line 113: /// Number of connections rejected due to timeout > Add terminating period (nit). Done http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.cpp File be/src/rpc/TAcceptQueueServer.cpp: http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/rpc/TAcceptQueueServer.cpp@220 PS8, Line 220: } catch (string s) { > I know this isn't part of this change, but can we really ever get string ex Good question. I am not certain about string exceptions, but the intent here seems to be to catch non-thrift exceptions. http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/12579/8/be/src/service/impala-server.cc@241 PS8, Line 241: "the queue before we time it out and reject the connection request. A value of 0 " > In the flags that configure the accept queue we call it "the post-accept, p Changed the wording. Please let me know if this looks better. http://gerrit.cloudera.org:8080/#/c/12579/8/tests/custom_cluster/test_frontend_connection_limit.py File tests/custom_cluster/test_frontend_connection_limit.py: http://gerrit.cloudera.org:8080/#/c/12579/8/tests/custom_cluster/test_frontend_connection_limit.py@89 PS8, Line 89: raise ImpalaBeeswaxException(e.message, e) > I am confused about this. Why do we raise an exception here? I see that th We need to catch the exception to close() the session so that the query gets unregistered, and re-raise the exception. Otherwise, the async query hangs around and makes the test fail. The comment in the above line tries to allude to that. Please let me know if I should expand the comment. -- 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: 8 Gerrit-Owner: Zoram Thanga <zo...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Mar 2019 21:34:14 +0000 Gerrit-HasComments: Yes
