John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17144 )
Change subject: IMPALA-10551: Add result sink support for external frontends ...................................................................... Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/17144/6/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/17144/6/be/src/runtime/coordinator.cc@796 PS6, Line 796: // All instances must have reported their final statuses before finalization, which is a : // post-condition of Wait. Result sink file clean up is the responsibility of the : // external frontend > This is a copy/paste from FinalizeHdfsDml(), so the second sentence doesn't Done http://gerrit.cloudera.org:8080/#/c/17144/6/be/src/runtime/coordinator.cc@802 PS6, Line 802: RETURN_IF_ERROR(UpdateExecState(Status::OK(), nullptr, FLAGS_hostname)); > If there is an error from execution, it would show up here and this would r I agree that retry_failed_queries might be problematic and we might want to recommend that an external frontend not enable the feature. If I am reading the coordinator.cc code correct though, we do not retry queries with a result sink. Are there other areas I should be concerned about? I see some usage in query-driver.cc http://gerrit.cloudera.org:8080/#/c/17144/6/be/src/runtime/coordinator.cc@807 PS6, Line 807: > Nit: This "0" is the table id. I'm guessing 0 is a special constant for the I moved it to a named constant within this method since it is the only usage of it. I can also move it to DescriptorTbl if your prefer it there. http://gerrit.cloudera.org:8080/#/c/17144/6/be/src/runtime/coordinator.cc@810 PS6, Line 810: result_sink_table_id, obj_p > I think this should be "0" (i.e. the special table id used in the CreateHdf I removed the table_id portion from this message/ -- To view, visit http://gerrit.cloudera.org:8080/17144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I024bf41d77bb81f1ab0debdbd31ec3687c83f072 Gerrit-Change-Number: 17144 Gerrit-PatchSet: 7 Gerrit-Owner: John Sherman <j...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: John Sherman <j...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Fri, 12 Mar 2021 23:43:05 +0000 Gerrit-HasComments: Yes
