Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/14824 )
Change subject: IMPALA-9199: Add support for single query retries on cluster membership changes ...................................................................... Patch Set 9: (8 comments) Still going through this, but some high level thoughts (and a few nit-picks I happened to notice already) http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/coordinator.cc@922 PS9, Line 922: parent_query_driver_->TryQueryRetry(parent_request_state_, &retryable_status)); So obviously the circular dependency here between QueryDriver, ClientRequestState, and Coordinator is unfortunate. It probably works as is, and it might be difficult to get rid of without significant code restructuring, but I'm concerned its confusing/brittle (eg. this call results in us taking ClientRequestState::lock_ on the ReportExecStatus rpc thread) and so I've been thinking through the options. Would it be possible to instead have the QueryDriver wait on the coordinator to finish and then check its status and decide whether to retry then? One problem is the QueryDriver needs to know not just if the query hit an error but if the error was something retryable, but we could do something like have the coordinator remember any nodes it blacklists and expose that info to the QueryDriver. Another issue is that it means we won't start the retry quite as quickly, since we have to wait for the QueryDriver to notice the error, but that might be fine - its already the case that Coordinator::Wait() will return immediately after an error is reported, without waiting for other backends to be cancelled, see HandleExecStateTransition()->CancelBackends()->backend_exec_complete_barrier_->NotifyRemaining(). The worse case is when the blacklisting info and the error status don't arrive in the same report, eg. the call to TryQueryRetry below, but that should be rare since the error status will have been generated at the same time as the AuxErrorInfo, so you just have to get pretty unlucky with the timing of when the report is generated. Maybe the bigger issue is I'm not sure its easy to do with the way the code is set up. I was trying to trace through the various Wait()/WaitAsync()/BlockOnWait() whatever calls but I'm not sure how that all works now. http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/coordinator.cc@1060 PS9, Line 1060: ExecEnv::GetInstance()->cluster_membership_mgr()->BlacklistExecutor( This of course doesn't actually guarantee that the retried query won't be scheduled on the executor that gets blacklisted, eg. if it takes longer for the query to get through admission control again than the blacklist timeout. We might want to consider doing something like passing through a list of executors to reschedule on, to avoid having queries repeatedly fail in the same way. On the other hand, the blacklist timeout was designed to be longer than it should take the statestore to notice the executor is down, so in theory if the executor really is down maybe it doesn't matter. Probably not necessary to do anything different for this patch, though, just wanted to point this out. http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/query-driver.h File be/src/runtime/query-driver.h: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/runtime/query-driver.h@113 PS9, Line 113: (2) the : /// query has already been retried not sure what this is supposed to mean http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/client-request-state.h File be/src/service/client-request-state.h: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/client-request-state.h@84 PS9, Line 84: UNKNOWN I don't think this is used anywhere? http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc File be/src/service/impala-hs2-server.cc: http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc@148 PS9, Line 148: unique_ptr<TExecRequest> exec_request = make_unique<TExecRequest>(); I don't think this is used anywhere? http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc@763 PS9, Line 763: ClientRequestState* request_state = nullptr; So I'm wondering if its safe to no longer have any shared_ptr here and in the other places in this file - what's to stop the QueryDriver from getting deleted by Unregister while this function is executing, which would make this pointer no longer valid? http://gerrit.cloudera.org:8080/#/c/14824/9/be/src/service/impala-hs2-server.cc@1039 PS9, Line 1039: // If the query was retried, fetch the profile for the most recent attempt of the query I think its definitely necessary that we provide a way for all relevant profiles to be accessed through HS2, not just the webserver. This is one case where the retries basically just can't be "transparent", eg. in your current solution clients get the confusing behavior of requesting a profile for a particular query_id and getting back a profile with a different query_id listed, and we should think carefully about the right behavior to make it as easy as possible for clients. Some options: - Return the profile corresponding to the provided query_id. Clients would need to follow the "Retried Query Id" in the profile to get the profile of the retry. - Return all profiles for all retries together in a single call, possibly requiring a flag like "get_all_retries" to be set on the request. http://gerrit.cloudera.org:8080/#/c/14824/9/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/14824/9/common/thrift/ImpalaService.thrift@523 PS9, Line 523: branch typo -- To view, visit http://gerrit.cloudera.org:8080/14824 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2e4a0e72a9bf8ec10b91639aefd81bef17886ddd Gerrit-Change-Number: 14824 Gerrit-PatchSet: 9 Gerrit-Owner: Sahil Takiar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Sahil Takiar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Tue, 07 Apr 2020 19:58:08 +0000 Gerrit-HasComments: Yes
