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 3: (10 comments) Thanks for the review. Uploading PS 4 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 avo I've thought about this a bit, and came up with this value which I think is at the threshold of tolerance for a user trying to run an interactive query. But 300s is fine for me to. 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 parame Done http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@193 PS3, Line 193: 0LL > nit: 'LL' not needed. Done 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) { Done 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 ? Done 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 ? Potentially, if we're constantly maxing out fe threads and new connections keep coming. I can reduce the log frequency. Let me know if you feel strongly about this. 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 ? I haven't seen a 'simple' way to do this, but definitely would like to add more info about the client as well as 'which' server this is, e.g., beeswax or hs2. 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 Tried this, but since unique_ptr is not copyable compilation fails at line 242 (calling a destroyed object). Tried to massage it a bit, but readability gets compromised. Will stick with shared_ptr for now. The shared_ptr gets moved to the SetupConnection thread anyway so hopefully this is not too obscure. 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) { Done http://gerrit.cloudera.org:8080/#/c/12579/3/be/src/rpc/TAcceptQueueServer.cpp@261 PS3, Line 261: MILLIS_PER_SEC; > long line. Done -- 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: Fri, 01 Mar 2019 00:38:37 +0000 Gerrit-HasComments: Yes