Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15737 )
Change subject: IMPALA-9653: Impala shouldn't create/remove staging directory during transactional INSERTs ...................................................................... Patch Set 1: Code-Review+2 (2 comments) http://gerrit.cloudera.org:8080/#/c/15737/1/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/15737/1/be/src/runtime/coordinator.cc@760 PS1, Line 760: return return_status; I am slightly worried about the early delete in a finalize function, as someone may add some more logic to the end. My suggestion is to extract the end of the function to something like Status DeleteQueryLevelStagingDir and call it only for the non-transactional case. http://gerrit.cloudera.org:8080/#/c/15737/1/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/15737/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1703 PS1, Line 1703: finalizeParams.setStaging_dir( : hdfsTable.getHdfsBaseDir() + "/_impala_insert_staging"); I wonder whether this thrift is meaningful at all - we have the same string baked in at https://github.com/apache/impala/blob/master/be/src/exec/hdfs-table-sink.cc#L125 -- To view, visit http://gerrit.cloudera.org:8080/15737 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6a3a502fb3b3cfe7a68323d3b0145e5fb149460 Gerrit-Change-Number: 15737 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Wed, 15 Apr 2020 15:13:50 +0000 Gerrit-HasComments: Yes
