Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12049 )
Change subject: IMPALA-4555: Make QueryState's status reporting more robust ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/coordinator-backend-state.cc@299 PS1, Line 299: report_seq_no <= instance_stats->last_report_seq_no_ || instance_stats->done_) { > Wouldn't this be a problem for intermediate report ? If the coordinator is You're right that this approach may cause non-idempotent parts of the report to be applied twice (at the moment, only the error log). As discussed offline, only bumping the seq no on the executor side if the report was successful doesn't work - since we'll wait 5s+ before sending the new report, it will contain new info that we do want to have applied. I also looked into the idea of just deferring sending the error log until the final message. Currently, the error log is only exposed by the beeswax call get_log(). I don't know if this is commonly called by users in the middle of query execution (the shell calls it, but only at the end of the query), but going forward we probably want to expose this in more ways (eg. as part of the runtime profile), and I think it's a better user experience to make the errors accessible as soon as possible, for example to help identify issues with queries that are very long running or appear to hang. Further, there are other non-idempotent things that we may want to include (eg. it would be nice to have the dml stats incrementally updated instead of just at the end), so for these reasons I think its worth trying to come up with a solution for this now. The solution I put together separates the non-idempotent parts of the report out into a 'stateful' report. If the report rpc fails, each fragment instance stores the stateful report it had generated, and adds it to the next report associated with its original sequence number. This ensures that the coordinator will only apply stateful portions of the report for sequence numbers it hasn't seen yet. Once a report is successful, all the stored stateful reports are cleared. Downsides to this approach vs. just sending the error log with the final report: - Copying the stateful report into the fragment instance's state takes time and adds memory usage and sending multiple stateful reports increases the size of the report rpc and puts more load on the coordinator. These issues will only occur if a report rpc actually fails, which is hopefully rare, but if things are already failing this may bog the system down even more. - Additional code complexity. http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/fragment-instance-state.h@99 PS1, Line 99: void SetFinalReportSent() { final_report_sent_ = true; } > May help to add comments on the expected caller and on when this is expecte Done http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.h@417 PS1, Line 417: (bool final > Comment on the input parameter. Done http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc@58 PS1, Line 58: DEFINE_int32(status_report_max_failures, 3, : "Max number of consecutive failed status reports to allow before cancelling"); > Actually, max_retries seems to be safer in the sense that it guarantees a m Done http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc@324 PS1, Line 324: Try to send the RPC 3 times before failing. Sleep for 100ms between retries. > May need to be updated. Done http://gerrit.cloudera.org:8080/#/c/12049/1/be/src/runtime/query-state.cc@368 PS1, Line 368: fis_map_[id]->runtime_state()->ClearUnreportedErrors(); > There is a race here: we may be clearing newly added errors we added after Done -- To view, visit http://gerrit.cloudera.org:8080/12049 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6007013fc2c9e8eeba11b752ee58fb3038da971 Gerrit-Change-Number: 12049 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Fri, 14 Dec 2018 23:38:08 +0000 Gerrit-HasComments: Yes
