Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator ...................................................................... Patch Set 4: (10 comments) http://gerrit.cloudera.org:8080/#/c/12299/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12299/4//COMMIT_MSG@30 PS4, Line 30: thread pool instead of a single thread?) not too concerned about this -- average query concurrency is going to be in the hundreds, maximum, and looping over even a few thousand queries every few seconds is not going to be a tax. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.h@140 PS4, Line 140: If nit: lower case 'i' http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/runtime/coordinator.cc@750 PS4, Line 750: if (exec_rpcs_complete_barrier_ == nullptr this is racy since exec_rpcs_complete_barrier_ is changed from nullptr to a valid pointer without any locking. Could we change this to not be a scoped_ptr but instead just initialized in the ctor to avoid this? http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@215 PS4, Line 215: DEFINE_int32(hung_backend_check_interval_s, 30, "The time, in seconds, to wait between " instead of introducing a new flag, can we just set this based on the configured kill timeout? eg we can just say that we always want to kill a query within 10% of the target kill timeout, and set this to 0.1*FLAGS_status_report_interval_ms. That way we don't have two flags that have some confusing interaction. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@398 PS4, Line 398: periodic status reporting is enabled unrelated to this patch, but why is this even an option? shouldn't the backends _always_ report status? Can we deprecate this configuration? http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2226 PS4, Line 2226: int64_t max_lag_ms = GetMaxReportRetryMs(); Instead of forcing that the coordinator and executors all agree on this configuration, would it be easy enough to plumb it through as part of the RPC that starts execution for a given query? if it's not trivial, i'm fine with this as is. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2227 PS4, Line 2227: LOG(INFO) << "Queries will be cancelled if a backend has not reported its status in " : << "more than " << max_lag_ms << "ms."; don't we already log all the flags at startup? I think it'd be better to avoid an additional INFO if it's already logged elsewhere. Maybe change to VLOG(1)? http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2237 PS4, Line 2237: Offer I noticed that 'Offer' seems to call to BlockingPut under the covers, which means this might have to acquire a lock and wait. It's unlkely since cancellation_thread_pool_ has a very long queue length, but still gives me the heebie-jeebies, because this lambda is executed while holding a shard lock in ShardedQueryMap. Instead, maybe worth putting a vector<shared_ptr<ClientRequestState>> to_cancel outside the DoFuncForAllEntries call, and pushing the request_state onto that vector here, and then putting it onto the cancellation pool following that. http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2288 PS4, Line 2288: // The max time that each retry takes is FLAGS_backend_client_rpc_timeout_ms. : // The wait time before a retry is: : // (<num consecutive failed reports> + 1) * FLAGS_status_report_interval_ms : // so max possible wait time is: : // (1 + 2 + ... + <max failed reports>) * FLAGS_status_report_interval_ms : // and 1 + 2 + ... + n = (n * (n + 1)) / 2 are these flags documented and user-settable? Is it too late to change the flag to just be a max retry time directly, rather than a count of retries? retry counts are complicated because, as you are showing here, they have a cross-flag interaction with the timeout, and also are going to have very different behavior depending on whether the call took a long time (a timeout) vs a short time (a service queue overflow) http://gerrit.cloudera.org:8080/#/c/12299/4/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: http://gerrit.cloudera.org:8080/#/c/12299/4/common/thrift/generate_error_codes.py@403 PS4, Line 403: 3s ms? -- To view, visit http://gerrit.cloudera.org:8080/12299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987 Gerrit-Change-Number: 12299 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 21 Mar 2019 04:53:49 +0000 Gerrit-HasComments: Yes
