Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15931 )
Change subject: IMPALA-9711: incrementally update aggregate profile ...................................................................... Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/15931/10/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: http://gerrit.cloudera.org:8080/#/c/15931/10/be/src/runtime/coordinator-backend-state.cc@539 PS10, Line 539: if (instance_stats.exec_stats_up_to_date_) continue; // Already applied. I'm thinking through this check and wondering if it interacts with finalize/done_. For V1, if this is true, then finalize = true or done_ = true, because that is the only time we call UpdateExecStats(). That sounds right to me, because if we called UpdateExecStats() with either of those set, then the completion time is set. For V2, we call UpdateExecStats() even when done_=false and finalize=false, so exec_stats_up_to_date_ can be true even though we haven't set the completion time. I think for done_ that is ok, because to transition from done_=false to done_=true involves a new status update. For finalize, there may not be a new status update, so I think we might not update the completion time correctly. -- To view, visit http://gerrit.cloudera.org:8080/15931 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib03e79a40a33d8e74464640ae5f95a1467a6713a Gerrit-Change-Number: 15931 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 30 Sep 2020 17:37:43 +0000 Gerrit-HasComments: Yes
