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 3: (10 comments) http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp File be/src/rpc/TAcceptQueueServer.cpp: http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@28 PS3, Line 28: 120, Will it be safer to have a higher default timeout (e.g. 300 seconds) to avoid users' complaint given the current behavior is that we don't have a timeout ? Please also add a remark that setting it to 0 means there is no timeout. http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@151 PS3, Line 151: const string& error) nit: we tend to put the constant parameters followed by non-constant parameters. http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@193 PS3, Line 193: 0LL nit: 'LL' not needed. http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@196 PS3, Line 196: if (entry->expiration_time_) nit: if (entry->expiration_time_ != 0) { http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@197 PS3, Line 197: wait_time = entry->expiration_time_ - MonotonicMillis(); Shouldn't this be inside the while loop below ? http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@199 PS3, Line 199: LOG(INFO) << "All " << maxTasks_ << " server threads are in use. " : << "Waiting for " << wait_time << " msecs."; Will this lead to log spam ? http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@204 PS3, Line 204: Timing out connection request."; Any chance we can print some details (e.g. IP address, port) here ? http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@239 PS3, Line 239: shared_ptr This can be "unique_ptr" now that TTransport lives inside TAcceptQueueEntry. This makes ownership easier to understand. http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@260 PS3, Line 260: if (FLAGS_accepted_cnxn_timeout) nit: if (FLAGS_accepted_cnxn_timeout != 0) { http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@261 PS3, Line 261: MILLIS_PER_SEC; long line. -- 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: 3 Gerrit-Owner: Zoram Thanga <zo...@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: Wed, 27 Feb 2019 22:26:02 +0000 Gerrit-HasComments: Yes
