Sailesh Mukil has posted comments on this change. ( http://gerrit.cloudera.org:8080/10158 )
Change subject: IMPALA-5384, part 2: Simplify Coordinator locking and clarify state ...................................................................... Patch Set 20: Code-Review+2 (4 comments) Thanks! This LGTM, +2-ing it since it's already gone through multiple rounds of review. All comments are just responses for discussion's sake, and don't require any code changes. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.h@325 PS18, Line 325: rySummary() and > ExecState is a scalar (enum). We pass scalar by value - no reason to have i I agree but on the other hand, keeping it const would help by avoiding accidental bugs such as overwriting the exec state in future changes, so it's better just to define the read only contract upfront. Also agree with no need for a ref. http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h File be/src/runtime/coordinator.h: http://gerrit.cloudera.org:8080/#/c/10158/14/be/src/runtime/coordinator.h@279 PS14, Line 279: > Cancel() is not called for (execution) errors. Thats part of the point of t I think I meant CancelBackends() as the NotifyAll() lives there, but the comment you updated it to is much clearer. Thanks. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc@457 PS18, Line 457: ret_status > exec_status_ is protected by the lock, so the copy is necessary. Fair point, I missed that. http://gerrit.cloudera.org:8080/#/c/10158/18/be/src/runtime/coordinator.cc@476 PS18, Line 476: DCHECK(exec_status_.ok()) << exec_status_; > My thinking was that it would be good to at least log the error, which is w Maybe we can have a larger discussion later, and leave this as it is now. But just as someone who frequents the logs to debug issues, if I see an error for a query, I would assume by default that the query failed, which could lead to some confusion, if it in fact didn't fail. Maybe a solution would be to meet in the middle and tag log messages such as this with something like "<fragment> failed but query succeeded", etc. But we don't need to have that convo in this patch. -- To view, visit http://gerrit.cloudera.org:8080/10158 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1abdfd02163f9356c59d470fe1c64ebe012a9e8e Gerrit-Change-Number: 10158 Gerrit-PatchSet: 20 Gerrit-Owner: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 11 May 2018 21:11:01 +0000 Gerrit-HasComments: Yes
