[Impala-ASF-CR] IMPALA-7800: Reject new connections after --fe service threads

["Zoram Thanga (Code Review)" <ge...@cloudera.org>]

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 <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Mar 2019 00:38:37 +0000
Gerrit-HasComments: Yes