You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Marshall (Code Review)" <ge...@cloudera.org> on 2019/01/29 23:03:31 UTC
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12299
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in greater than a
configurable time.
It introduces two new flags:
--hung_query_check_interval_s: the frequency that the thread will wake
up to do the checking
--max_report_lag_s: the amount of time to wait for a report from a
backend before cancelling the query
TODO:
- Run real cluster tests to determine appropriate default values for
the flags and how scalable this approach is (eg. should we use a
thread pool instead of a single thread?)
- Write functional tests once the appropriate mechanisms are in place
to simulate errors (IMPALA-8138)
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
5 files changed, 64 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/1
--
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: newchange
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12299/8/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:
http://gerrit.cloudera.org:8080/#/c/12299/8/tests/custom_cluster/test_rpc_timeout.py@44
PS8, Line 44: e
flake8: E501 line too long (103 > 90 characters)
--
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: 8
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Apr 2019 23:39:35 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2448/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Mar 2019 22:03:57 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2976/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 11
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Apr 2019 20:36:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Mar 2019 04:53:49 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 12: Code-Review+2
carrying forward
--
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: 12
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Apr 2019 20:32:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2847/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 8
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 20 Apr 2019 00:21:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@215
PS10, Line 215: DEFINE_int32(status_report_interval_ms, 5000, "(Advanced) Interval between profile "
> (Advanced) Interval ...
Done
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@218
PS10, Line 218: DEFINE_int32(status_report_max_retry_s, 600, "(Advanced) Max amount of time in seconds "
> (Advanced) Max ...
Done
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@220
PS10, Line 220: "--status_report_interval_ms. Effective only if --status_report_interval_ms > 0.");
> Effective only if status_report_interval_ms > 0
Done
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@221
PS10, Line 221: DEFINE_int32(status_report_cancellation_padding, 20, "(Advanced) The coordinator will "
> (Advanced) The ..
Done
--
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: 11
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Apr 2019 19:48:06 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator-backend-state.cc@174
PS7, Line 174: last_report_time_ms_ = GenerateReportTimestamp();
: exec_complete_barrier->Notify();
> Does it implicitly rely on the LIFO order for local variables' destruction
Done
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator.cc@549
PS7, Line 549: DCHECK_
> DCHECK_LE
Done
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator.cc@664
PS7, Line 664: DCHECK_
> DCHECK_LE
Done
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(status_report_interval_ms, 5000,
> right, I think I meant 0.1 * GetMaxReportRetryMs(). ie we need to check muc
Done
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2288
PS4, Line 2288: int64_t cpu_limit_s = crs->query_options().cpu_limit_s;
: int64_t cpu_limit_ns = cpu_limit_s * 1000'000'000L;
: if (cpu_limit_ns > 0 && cpu_time_ns > cpu_limit_ns) {
: Status err = Status::Expected(TErrorCode::CPU_LIMIT_EXCEEDED,
: PrintId(crs->query_id()), PrettyPrinter::Print(cpu_limit_s, TUnit::TIME_S));
: VLOG_QUERY << err.msg().msg();
> The executor's reporting path currently can tolerate up to FLAGS_status_rep
As discussed offline, I have a preference for the alternative option you list, as it minimizes the probability that a user can set the flags in a way that is fundamentally wrong while still leaving a lot of room to tune the behavior.
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/service/impala-server.cc@2258
PS7, Line 2258: 64_t lag_time_ms
> per comment elsewhere: don't we need to wake up more often than the timeout
Done
--
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: 8
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 19 Apr 2019 23:38:26 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h@137
PS2, Line 137: boost::lock_guard<boost::mutex> l(lock_);
> Is that something we can rely on in all possible environments in which Impa
Yes, I suppose it may not hold true for all architecture. It generally holds for x86-64 architecture. That said, Impala is quite x86 centric although there were works in the past to port it to Power architecture.
Anyhow, this is not important unless we see any contention during perf testing.
--
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: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Feb 2019 00:57:19 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 9:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/12299/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12299/8//COMMIT_MSG@34
PS8, Line 34: 'REPORT_EXEC_STATUS_DELAY:JITTER@1000'
> Stale ?
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/runtime/query-state.cc@400
PS8, Line 400: we set backend rpc timeouts for kr
> Can you please leave a TODO on rethinking the timeout period for RPCs in ge
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/runtime/query-state.cc@414
PS8, Line 414: repor
> spent
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@217
PS8, Line 217: "report is sent.");
> and only the final report is sent.
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@218
PS8, Line 218: 600
> To avoid regression, let's be more conservative and set it to be at least 3
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@221
PS8, Line 221: The coordinator will wait "
: "--status_report_max_retry_s * (1 + --status_report_cancellation_padding / 100) "
: "without receiving a status report before deciding that a backend is unresponsive
> May be clearer to include the formula you have in the commit message.
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@421
PS8, Line 421: if (FLAGS_abort_on_config_error) {
> May wanna check if --status_report_cancellation_padding is set to something
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@2248
PS8, Line 2248: // this thread.
> DCHECK_GT(max_lag_ms, 0);
Done
http://gerrit.cloudera.org:8080/#/c/12299/8/tests/custom_cluster/test_rpc_timeout.py
File tests/custom_cluster/test_rpc_timeout.py:
http://gerrit.cloudera.org:8080/#/c/12299/8/tests/custom_cluster/test_rpc_timeout.py@44
PS8, Line 44:
> flake8: E501 line too long (103 > 90 characters)
Done
--
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: 9
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Apr 2019 20:20:44 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2447/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Mar 2019 21:55:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1924/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Wed, 30 Jan 2019 01:35:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 5:
(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: - Ran the stress test on tpch 500 on a 10 node cluster for 1000
> not too concerned about this -- average query concurrency is going to be in
Done
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'
Done
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: return 0;
> this is racy since exec_rpcs_complete_barrier_ is changed from nullptr to a
Done
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(status_report_interval_ms, 5000,
> instead of introducing a new flag, can we just set this based on the config
I think you're saying to set this to 1.1 * GetMaxReportRetryMs()?
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@398
PS4, Line 398:
> unrelated to this patch, but why is this even an option? shouldn't the back
Of course, even in this configuration, backends will report their final status. This allows users to significantly reduce the load on the coordinator at the cost of not being able to monitor the progress of in-flight queries.
I don't have strong feelings about whether or not this should be deprecated. I'm not aware of any users who rely on this. I spent a little time in 'git blame' and its been an option for years but there doesn't seem to be a documented reason for it.
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2226
PS4, Line 2226: }
> Instead of forcing that the coordinator and executors all agree on this con
Sure, that's pretty easy.
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2227
PS4, Line 2227: }
:
> don't we already log all the flags at startup? I think it'd be better to av
Done
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2237
PS4, Line 2237: ate->
> I noticed that 'Offer' seems to call to BlockingPut under the covers, which
Done
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2288
PS4, Line 2288: return Status::OK();
: }
:
: void ImpalaServer::ExpireQuery(ClientRequestState* crs, const Status& status) {
: DCHECK(!status.ok());
: cancellation_thread_pool_->Offer(
> are these flags documented and user-settable? Is it too late to change the
Yes, all of these flags are documented and settable.
The flag 'status_report_max_retries' shipped in 3.2, though it could of course be deprecated if we feel a max retry time is a significant improvement.
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: 3m
> ms?
Done
--
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: 5
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 25 Mar 2019 23:00:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 12: Verified+1
--
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: 12
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 30 Apr 2019 00:53:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator-backend-state.cc@174
PS7, Line 174: const auto trigger = MakeScopeExitTrigger(
: [this]() { last_report_time_ms_ = GenerateReportTimestamp(); });
Does it implicitly rely on the LIFO order for local variables' destruction in C++ so "notifier" won't be destroyed before "trigger" ? Otherwise, we could have notified 'exec_complete_barrier' without setting 'last_report_time_ms'.
I may be paranoid but can you please add a comment about this so future changes won't accidentally swap their orders ? I suppose we can skip the use of NotifyBarrierOnExit here and just notify 'exec_complete_barrier' in 'trigger' after setting 'last_report_tims_ms_'.
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator.cc@549
PS7, Line 549: DCHECK(
DCHECK_LE
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/runtime/coordinator.cc@664
PS7, Line 664: DCHECK(
DCHECK_LE
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@398
PS4, Line 398:
> k. We could deprecate it in a separate patch I suppose. Fine with this as i
As Thomas said, this config option was useful when users faced instability with large Impala cluster due to IMPALA-2990 (which this patch is fixing). We can consider deprecating this flag once we are convinced that this patch makes enough improvement that the instability with large cluster is mostly mitigated.
The periodic reporting matters for long running queries. The default reporting interval of 5 seconds could be longer than most of the short running query's duration. For long running queries, it probably doesn't really matter to get a report every 5 seconds vs say 30s or 60s. If we ever deprecate this flag, please consider bumping the the default reporting interval to something larger.
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2288
PS4, Line 2288: return Status::OK();
: }
:
: void ImpalaServer::ExpireQuery(ClientRequestState* crs, const Status& status) {
: DCHECK(!status.ok());
: cancellation_thread_pool_->Offer(
> k, would be interested what Michael thinks about that idea
The executor's reporting path currently can tolerate up to FLAGS_status_report_max_retries number of failures before giving up and declaring the coordinator as inaccessible. It's intentionally not a time-based approach and instead relies on the periodic reporting to handle the retry.
Stepping back a bit, my understanding for why we want to approximate the maximum retry time is that the coordinator may not want to prematurely cancel a query due to a missing report from an executor if it knows that the executor *may* still be retrying to send a report. However, as pointed out elsewhere by Todd, if an Impala can experience random pauses or random scheduling delay for the reporting thread, no amount of wait time will guarantee that an executor is not in the middle of retrying to send a report when the query is cancelled. I suppose that's not an end-goal we try to achieve.
This prompts me to think whether it's any different if we just do the following:
- expose a coordinator's maximum tolerable lag time as a knob set to arbitrary number
- expose an executor's maximum tolerable retry time as a knob set to some arbitrary number
- deprecate FLAGS_status_report_max_retries
An alternative would be to:
- expose an executor's maximum tolerable retry time as a knob set to some arbitrary number
- deprecate FLAGS_status_report_max_retries
- set coordinator's maximum tolerable lag time as (100 + x)% of the maximum tolerable retry time where x *may be* tunable in case the default value causes any trouble.
Todd / Thomas, what do you think ?
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/service/impala-server.cc@218
PS7, Line 218: DEFINE_int32(status_report_max_retries, 3,
> Is our timeout too aggressive here? If one of the backends goes into some k
I agree that the coordinator should be more lenient in handling lost reports to avoid false positive. On the other hand, no amount of timeout can save us from random pauses in Impalad so we should work on fixing IMPALA-2800 if possible at all.
--
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: 7
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 02 Apr 2019 01:09:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#7).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
The timeout is calculated as the longest a backend will attempt to
retry sending status reports before giving up and cancelling itself.
With the default flags, this timeout is about 15 minutes.
The thread wakes up at an interval of the calculated timeout + 10%
TODO:
- Write functional tests once the appropriate mechanisms are in place
to simulate errors (IMPALA-8138)
Testing:
Ran tests on a 10 node cluster loaded with tpch 500:
- Ran the stress test for 1000 queries with the debug actions:
'REPORT_EXEC_STATUS_SEND:FAIL@0.1|REPORT_EXEC_STATUS_RECV:FAIL@0.1'
Prior to this patch, this setup results in hanging queries. With
this patch, no hangs were observed.
- Ran perf tests with 4 concurrent streams, 3 iterations per query.
Found no change in performance.
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
10 files changed, 155 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/7
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 2:
Any update ?
--
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: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Feb 2019 23:26:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#5).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
The timeout is calculated as the longest a backend will attempt to
retry sending status reports before giving up and cancelling itself.
With the default flags, this timeout is about 15 minutes.
The thread wakes up at an interval of the calculated timeout + 10%
TODO:
- Write functional tests once the appropriate mechanisms are in place
to simulate errors (IMPALA-8138)
Testing:
- Ran the stress test on tpch 500 on a 10 node cluster for 1000
queries with the debug actions:
'REPORT_EXEC_STATUS_SEND:FAIL@0.1|REPORT_EXEC_STATUS_RECV:FAIL@0.1'
Prior to this patch, this setup results in hanging queries. With
this patch, no hangs were observed.
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
10 files changed, 155 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/5
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 5
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 8:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/12299/8//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12299/8//COMMIT_MSG@34
PS8, Line 34: 'REPORT_EXEC_STATUS_SEND:FAIL@0.1|REPORT_EXEC_STATUS_RECV:FAIL@0.1'
Stale ?
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/runtime/query-state.cc@400
PS8, Line 400: FLAGS_backend_client_rpc_timeout_ms
Can you please leave a TODO on rethinking the timeout period for RPCs in general. As we convert more of the RPCs from Thrift To KRPC, this flag's role has changed.
Historically, this is used for setting socket timeout with Thrift RPC so that an Impala thread will pop out of the Thrift RPC stack and check for cancellation.
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/runtime/query-state.cc@414
PS8, Line 414: spend
spent
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@217
PS8, Line 217: "reporting is disabled.");
and only the final report is sent.
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@218
PS8, Line 218: 180
To avoid regression, let's be more conservative and set it to be at least 300s, if not larger.
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@221
PS8, Line 221: The percent padding that a "
: "coordinator should wait to recieve a report after --status_report_max_retry_s "
: "before deciding that a backend is unresponsive and the query should be cancelled.
May be clearer to include the formula you have in the commit message.
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@421
PS8, Line 421: }
May wanna check if --status_report_cancellation_padding is set to something sane.
http://gerrit.cloudera.org:8080/#/c/12299/8/be/src/service/impala-server.cc@2248
PS8, Line 2248: * (1 + FLAGS_status_report_cancellation_padding / 100.0);
DCHECK_GT(max_lag_ms, 0);
--
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: 8
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 23 Apr 2019 22:34:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/4104/ DRY_RUN=false
--
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: 12
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Apr 2019 19:48:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
This patch adds two flags:
--status_report_max_retry_s: the maximum number of seconds a backend
will attempt to send status reports before giving up. This is used
in place of --status_report_max_retries which is now deprecated.
--status_report_cancellation_padding: the coordinator will wait
--status_report_max_retry_s *
(1 + --status_report_cancellation_padding / 100)
before concluding a backend is not responding and cancelling the
query.
Testing:
- Added a functional test that runs a query that is cancelled through
the new mechanism.
- Passed a full set of exhaustive tests.
Ran tests on a 10 node cluster loaded with tpch 500:
- Ran the stress test for 1000 queries with the debug actions:
'REPORT_EXEC_STATUS_DELAY:JITTER@1000'
Prior to this patch, this setup results in hanging queries. With
this patch, no hangs were observed.
- Ran perf tests with 4 concurrent streams, 3 iterations per query.
Found no change in performance.
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Reviewed-on: http://gerrit.cloudera.org:8080/12299
Reviewed-by: Thomas Marshall <tm...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/common/global-flags.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_rpc_timeout.py
12 files changed, 191 insertions(+), 47 deletions(-)
Approvals:
Thomas Marshall: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 13
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 10: Code-Review+2
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/runtime/query-state.cc@a444
PS10, Line 444:
May be nice to keep this part but I guess it's not as important now that IMPALA-6664 is fixed.
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@215
PS10, Line 215: DEFINE_int32(status_report_interval_ms, 5000, "Interval between profile reports in "
(Advanced) Interval ...
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@218
PS10, Line 218: DEFINE_int32(status_report_max_retry_s, 600, "Max amount of time in seconds for a "
(Advanced) Max ...
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@220
PS10, Line 220: "--status_report_interval_ms.");
Effective only if status_report_interval_ms > 0
http://gerrit.cloudera.org:8080/#/c/12299/10/be/src/service/impala-server.cc@221
PS10, Line 221: DEFINE_int32(status_report_cancellation_padding, 20, "The coordinator will wait "
(Advanced) The ..
--
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: 10
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 26 Apr 2019 21:47:34 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#11).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
This patch adds two flags:
--status_report_max_retry_s: the maximum number of seconds a backend
will attempt to send status reports before giving up. This is used
in place of --status_report_max_retries which is now deprecated.
--status_report_cancellation_padding: the coordinator will wait
--status_report_max_retry_s *
(1 + --status_report_cancellation_padding / 100)
before concluding a backend is not responding and cancelling the
query.
Testing:
- Added a functional test that runs a query that is cancelled through
the new mechanism.
- Passed a full set of exhaustive tests.
Ran tests on a 10 node cluster loaded with tpch 500:
- Ran the stress test for 1000 queries with the debug actions:
'REPORT_EXEC_STATUS_DELAY:JITTER@1000'
Prior to this patch, this setup results in hanging queries. With
this patch, no hangs were observed.
- Ran perf tests with 4 concurrent streams, 3 iterations per query.
Found no change in performance.
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/common/global-flags.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_rpc_timeout.py
12 files changed, 191 insertions(+), 47 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/11
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 11
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12299/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12299/1//COMMIT_MSG@29
PS1, Line 29: - Write functional tests once the appropriate mechanisms are in place
> I think you can "kill -STOP" an impalad running a query, and it will end up
That scenario is actually already handled prior to this patch by the statestore detecting the impalad is down.
This patch is for queries where the impalad doesn't fail but status reports aren't getting sent for some reason.
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/runtime/coordinator.cc@213
PS1, Line 213: }
> This could probably just use backend_states_.empty() instead. I don't mind
Done
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/runtime/coordinator.cc@744
PS1, Line 744: int64_t current_time = BackendState::GenerateReportTimestamp();
> It's confusing, as be/src/runtime/coordinator-backend-state.h has similarly
Yeah, this is subtle. Added a helper function to make it clearer/reduce the chances we mess this up in the future.
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/service/impala-server.cc@2212
PS1, Line 2212:
> Do we have a style to write
Done
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/service/impala-server.cc@2209
PS1, Line 2209: TNetworkAddress address;
: int64_t lag_time_ms = coord->GetMaxBackendStateLagMs(&address);
: if (lag_time_ms > FLAGS_max_report_lag_s * 1000) {
: cancellation_thread_pool_->Offer(
> If I were a user receiving this error, I'd immediately want to know which b
Done
--
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: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 30 Jan 2019 21:59:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/1939/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Wed, 30 Jan 2019 22:31:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2894/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 9
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Apr 2019 21:14:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2537/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 5
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 25 Mar 2019 23:42:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#3).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
The timeout is calculated as the longest a backend will attempt to
retry sending status reports before giving up and cancelling itself.
With the default flags, this timeout is about 15 minutes.
It introduces one new flag:
--hung_backend_check_interval_s: the frequency that the thread will
wake up to do the checking
TODO:
- Run real cluster tests to determine appropriate default values for
the flags and how scalable this approach is (eg. should we use a
thread pool instead of a single thread?)
- Write functional tests once the appropriate mechanisms are in place
to simulate errors (IMPALA-8138)
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/generate_error_codes.py
M tests/run-tests.py
10 files changed, 126 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/3
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 3:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12299/3/tests/run-tests.py
File tests/run-tests.py:
http://gerrit.cloudera.org:8080/#/c/12299/3/tests/run-tests.py@278
PS3, Line 278: #
flake8: E265 block comment should start with '# '
http://gerrit.cloudera.org:8080/#/c/12299/3/tests/run-tests.py@283
PS3, Line 283: #
flake8: E265 block comment should start with '# '
http://gerrit.cloudera.org:8080/#/c/12299/3/tests/run-tests.py@286
PS3, Line 286: #
flake8: E265 block comment should start with '# '
http://gerrit.cloudera.org:8080/#/c/12299/3/tests/run-tests.py@303
PS3, Line 303: #
flake8: E265 block comment should start with '# '
http://gerrit.cloudera.org:8080/#/c/12299/3/tests/run-tests.py@304
PS3, Line 304: #
flake8: E265 block comment should start with '# '
http://gerrit.cloudera.org:8080/#/c/12299/3/tests/run-tests.py@305
PS3, Line 305: #
flake8: E265 block comment should start with '# '
--
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: 3
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 15 Mar 2019 21:23:56 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#8).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
This patch adds two flags:
--status_report_max_retry_s: the maximum number of seconds a backend
will attempt to send status reports before giving up. This is used
in place of --status_report_max_retries which is now deprecated.
--status_report_cancellation_padding: the coordinator will wait
--status_report_max_retry_s *
(1 + --status_report_cancellation_padding / 100)
before concluding a backend is not responding and cancelling the
query.
Testing:
- Added a functional test that runs a query that is cancelled through
the new mechanism.
Ran tests on a 10 node cluster loaded with tpch 500:
- Ran the stress test for 1000 queries with the debug actions:
'REPORT_EXEC_STATUS_SEND:FAIL@0.1|REPORT_EXEC_STATUS_RECV:FAIL@0.1'
Prior to this patch, this setup results in hanging queries. With
this patch, no hangs were observed.
- Ran perf tests with 4 concurrent streams, 3 iterations per query.
Found no change in performance.
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/common/global-flags.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_rpc_timeout.py
12 files changed, 179 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/8
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#9).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
This patch adds two flags:
--status_report_max_retry_s: the maximum number of seconds a backend
will attempt to send status reports before giving up. This is used
in place of --status_report_max_retries which is now deprecated.
--status_report_cancellation_padding: the coordinator will wait
--status_report_max_retry_s *
(1 + --status_report_cancellation_padding / 100)
before concluding a backend is not responding and cancelling the
query.
Testing:
- Added a functional test that runs a query that is cancelled through
the new mechanism.
Ran tests on a 10 node cluster loaded with tpch 500:
- Ran the stress test for 1000 queries with the debug actions:
'REPORT_EXEC_STATUS_DELAY:JITTER@1000'
Prior to this patch, this setup results in hanging queries. With
this patch, no hangs were observed.
- Ran perf tests with 4 concurrent streams, 3 iterations per query.
Found no change in performance.
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/common/global-flags.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_rpc_timeout.py
12 files changed, 190 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/9
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 9
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#4).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
The timeout is calculated as the longest a backend will attempt to
retry sending status reports before giving up and cancelling itself.
With the default flags, this timeout is about 15 minutes.
It introduces one new flag:
--hung_backend_check_interval_s: the frequency that the thread will
wake up to do the checking
TODO:
- Run real cluster tests to determine appropriate default values for
the flags and how scalable this approach is (eg. should we use a
thread pool instead of a single thread?)
- Write functional tests once the appropriate mechanisms are in place
to simulate errors (IMPALA-8138)
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/generate_error_codes.py
9 files changed, 117 insertions(+), 13 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/4
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 10: Code-Review+1
rebased, carrying forward
--
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: 10
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Apr 2019 22:28:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 2:
(4 comments)
> Any update ?
Got distracted by stress test stuff, but yes I'll be updating this and my other pending krpc patch soon
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h@137
PS2, Line 137: boost::lock_guard<boost::mutex> l(lock_);
> In theory, this doesn't need any locking as read / write of a 64-bit word s
Is that something we can rely on in all possible environments in which Impala might run?
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h@153
PS2, Line 153: /// Returns a timestamp for use in tracking arrival of status reports.
> May help to state explicitly that this uses the monotonic time instead of U
Done
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.cc@89
PS2, Line 89: last_report_time_ms_ = GenerateReportTimestamp();
> Should this clock not be started until we have issued all the initial ExecF
Done
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator.cc@747
PS2, Line 747: for (BackendState* backend_state : backend_states_) {
> Is there any race with the init code which populates
> backend_states_ ? Shouldn't the detection be skipped until the
> backend_states_ be fully populated ?
This is handled by the use of 'backend_states_init_lock' above. Since we're moving to setting 'last_report_time' in Exec() instead of Init(), the new patch uses exec_rpcs_complete_barrier_ instead
>
> Does this handle the case in which a backend is done so no more new
> report will be received ?
Done
--
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: 2
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 26 Feb 2019 00:17:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h
File be/src/runtime/coordinator-backend-state.h:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h@137
PS2, Line 137: boost::lock_guard<boost::mutex> l(lock_);
In theory, this doesn't need any locking as read / write of a 64-bit word should be atomic as long as its' 64-bit aligned.
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.h@153
PS2, Line 153: /// Returns a timestamp for use in tracking arrival of status reports.
May help to state explicitly that this uses the monotonic time instead of Unix time.
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator-backend-state.cc@89
PS2, Line 89: last_report_time_ms_ = GenerateReportTimestamp();
Should this clock not be started until we have issued all the initial ExecFInstance() RPC ? I can see there may be cases in which starting a query in a large cluster can take a bit of time so we may be more prone to false positive.
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/runtime/coordinator.cc@747
PS2, Line 747: for (BackendState* backend_state : backend_states_) {
Is there any race with the init code which populates backend_states_ ? Shouldn't the detection be skipped until the backend_states_ be fully populated ?
Does this handle the case in which a backend is done so no more new report will be received ?
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/2/be/src/service/impala-server.cc@215
PS2, Line 215: (max_report_lag_s, 30,
Shouldn't the lag be dependent on the reporting interval ? Ideally, we should mark the query as hung after x * reporting interval has elapsed without receiving a report.
In addition, we need to handle the case of periodic reporting being disabled by the user. In that case, we probably need to disable this detection.
--
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: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Comment-Date: Thu, 31 Jan 2019 20:18:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 9: Code-Review+1
--
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: 9
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Apr 2019 18:43:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Todd Lipcon, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#10).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in a certain amount of
time.
This patch adds two flags:
--status_report_max_retry_s: the maximum number of seconds a backend
will attempt to send status reports before giving up. This is used
in place of --status_report_max_retries which is now deprecated.
--status_report_cancellation_padding: the coordinator will wait
--status_report_max_retry_s *
(1 + --status_report_cancellation_padding / 100)
before concluding a backend is not responding and cancelling the
query.
Testing:
- Added a functional test that runs a query that is cancelled through
the new mechanism.
- Passed a full set of exhaustive tests.
Ran tests on a 10 node cluster loaded with tpch 500:
- Ran the stress test for 1000 queries with the debug actions:
'REPORT_EXEC_STATUS_DELAY:JITTER@1000'
Prior to this patch, this setup results in hanging queries. With
this patch, no hangs were observed.
- Ran perf tests with 4 concurrent streams, 3 iterations per query.
Found no change in performance.
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/common/global-flags.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/ImpalaInternalService.thrift
M common/thrift/generate_error_codes.py
M tests/custom_cluster/test_rpc_timeout.py
12 files changed, 190 insertions(+), 46 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/10
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 10
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho, Philip Zeyliger, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12299
to look at the new patch set (#2).
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
IMPALA-2990: timeout unresponsive queries in coordinator
The coordinator currently waits indefinitely if it does not receive a
status report from a backend. This could cause a query to hang
indefinitely in certain situations, for example if the backend decides
to cancel itself as a result of failed status report rpcs.
This patch adds a thread to ImpalaServer which periodically iterates
over all queries for which that server is the coordinator and cancels
any that haven't had a report from a backend in greater than a
configurable time.
It introduces two new flags:
--hung_query_check_interval_s: the frequency that the thread will wake
up to do the checking
--max_report_lag_s: the amount of time to wait for a report from a
backend before cancelling the query
TODO:
- Run real cluster tests to determine appropriate default values for
the flags and how scalable this approach is (eg. should we use a
thread pool instead of a single thread?)
- Write functional tests once the appropriate mechanisms are in place
to simulate errors (IMPALA-8138)
Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
---
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator-backend-state.h
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/service/impala-server.cc
M be/src/service/impala-server.h
M common/thrift/generate_error_codes.py
7 files changed, 87 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/12299/2
--
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: newpatchset
Gerrit-Change-Id: I196c8c6a5633b1960e2c3a3884777be9b3824987
Gerrit-Change-Number: 12299
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2924/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 10
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 25 Apr 2019 22:51:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Philip Zeyliger (Code Review)" <ge...@cloudera.org>.
Philip Zeyliger has posted comments on this change. ( http://gerrit.cloudera.org:8080/12299 )
Change subject: IMPALA-2990: timeout unresponsive queries in coordinator
......................................................................
Patch Set 1:
(5 comments)
Cool.
http://gerrit.cloudera.org:8080/#/c/12299/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12299/1//COMMIT_MSG@29
PS1, Line 29: - Write functional tests once the appropriate mechanisms are in place
I think you can "kill -STOP" an impalad running a query, and it will end up looking like this to the coordinator. Of course, maybe something else will timeout.
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/runtime/coordinator.cc@213
PS1, Line 213: backend_states_init_ = true;
This could probably just use backend_states_.empty() instead. I don't mind the extra variable, though. We have .empty() usages elsewhere.
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/runtime/coordinator.cc@744
PS1, Line 744: int64_t current_time = MonotonicMillis();
It's confusing, as be/src/runtime/coordinator-backend-state.h has similarly named variables that are in UnixMillis() and MonotonicMillis(). I think this is correct at the moment, which is good.
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/service/impala-server.cc@2212
PS1, Line 2212: true
Do we have a style to write
TerminatedByServer(...., true /* expire */)
?
http://gerrit.cloudera.org:8080/#/c/12299/1/be/src/service/impala-server.cc@2209
PS1, Line 2209: int64_t lag_time_ms = coord->GetMaxBackendStateLagMs();
: if (lag_time_ms > FLAGS_max_report_lag_s * 1000) {
: cancellation_thread_pool_->Offer(CancellationWork::TerminatedByServer(
: request_state->query_id(), Status("hung query"), true));
If I were a user receiving this error, I'd immediately want to know which backend failed. Let's include more information here, including the values of the flags, the max lag, and where the max lag was observed.
--
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: 1
Gerrit-Owner: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Comment-Date: Tue, 29 Jan 2019 23:31:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-2990: timeout unresponsive queries in coordinator
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 7:
(5 comments)
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(status_report_interval_ms, 5000,
> I think you're saying to set this to 1.1 * GetMaxReportRetryMs()?
right, I think I meant 0.1 * GetMaxReportRetryMs(). ie we need to check much more frequently than the timeout, or else we could overshoot it by a factor of 2
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@398
PS4, Line 398:
> Of course, even in this configuration, backends will report their final sta
k. We could deprecate it in a separate patch I suppose. Fine with this as is.
http://gerrit.cloudera.org:8080/#/c/12299/4/be/src/service/impala-server.cc@2288
PS4, Line 2288: return Status::OK();
: }
:
: void ImpalaServer::ExpireQuery(ClientRequestState* crs, const Status& status) {
: DCHECK(!status.ok());
: cancellation_thread_pool_->Offer(
> Yes, all of these flags are documented and settable.
k, would be interested what Michael thinks about that idea
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/service/impala-server.cc@218
PS7, Line 218: DEFINE_int32(status_report_max_retries, 3,
Is our timeout too aggressive here? If one of the backends goes into some kind of 15s process-wide pause (eg as we've seen in the past with tcmalloc releasing memory in https://issues.apache.org/jira/browse/IMPALA-2800) we'll end up cancelling. Am afraid that being too aggressive will end up cancelling queries incorrectly. Curious for Michael's thoughts
http://gerrit.cloudera.org:8080/#/c/12299/7/be/src/service/impala-server.cc@2258
PS7, Line 2258: max_lag_ms * 1.1
per comment elsewhere: don't we need to wake up more often than the timeout? otherwise we can overshoot by a factor of 2 here.
--
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: 7
Gerrit-Owner: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 26 Mar 2019 23:16:03 +0000
Gerrit-HasComments: Yes