Michael Ho 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 4: (9 comments) Looking good. Some more minor comments. http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/coordinator-backend-state.cc@344 PS4, Line 344: for (const auto& stateful_report : instance_exec_status.stateful_report()) { DCHECK_LE(stateful_report.report_seq_no(), report_seq_no()); http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/fragment-instance-state.h File be/src/runtime/fragment-instance-state.h: http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/fragment-instance-state.h@170 PS4, Line 170: recieved typo http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/fragment-instance-state.h@171 PS4, Line 171: stateful_reports_ nit: prev_stateful_reports_; http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/fragment-instance-state.cc@253 PS4, Line 253: if (final_report_saved_) { Please see comments below about some potential simplification we can do. http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/fragment-instance-state.cc@279 PS4, Line 279: {stateful_reports_.begin(), stateful_reports_.end()}; nit: indent 4 http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/fragment-instance-state.cc@298 PS4, Line 298: if (instance_exec_status.done()) { As discussed in person, this could have been simpler by not storing the final profile here. We just have to keep track of the fact that we don't need to bump the sequence number once the final report has been generated at least once. http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/query-state.h File be/src/runtime/query-state.h: http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/query-state.h@410 PS4, Line 410: report_ stale ? http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/query-state.h@411 PS4, Line 411: profiles_forest_' stale ? http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: http://gerrit.cloudera.org:8080/#/c/12049/4/be/src/runtime/query-state.cc@563 PS4, Line 563: FLAGS_status_report_interval_ms * (num_failed_reports_ + 1) Given this is quite clunky to write and used at more than one places, may be it makes sense to factor it into a function itself. This also makes changing the logic of sleep time (e.g. exponential backoff) in the future easier. -- 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: 4 Gerrit-Owner: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Comment-Date: Thu, 31 Jan 2019 19:46:51 +0000 Gerrit-HasComments: Yes
