John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/17145/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17145/1//COMMIT_MSG@22 PS1, Line 22: > Add co-author for Kurt Done http://gerrit.cloudera.org:8080/#/c/17145/2/be/src/exec/hdfs-table-sink.h File be/src/exec/hdfs-table-sink.h: http://gerrit.cloudera.org:8080/#/c/17145/2/be/src/exec/hdfs-table-sink.h@249 PS2, Line 249: // Stores the indices into the list of non-clustering columns of the target table that > Extra line Done http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.h File be/src/exec/hdfs-table-sink.h: http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.h@249 PS1, Line 249: // Stores the indices into the list of non-clustering columns of the target table that > delete Done http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc File be/src/exec/hdfs-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc@131 PS1, Line 131: staging_dir_ = Substitute("$0/_impala_insert_staging/$1", > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc@232 PS1, Line 232: void HdfsTableSink::BuildHdfsFileNames( > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc@506 PS1, Line 506: for (int j = 0; j < partition_key_expr_evals_.size(); ++j) { > line too long (91 > 90) Done http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc@549 PS1, Line 549: // partition_name_ss now holds the unique descriptor for this partition, > line too long (95 > 90) Done http://gerrit.cloudera.org:8080/#/c/17145/2/be/src/exec/hdfs-table-sink.cc File be/src/exec/hdfs-table-sink.cc: http://gerrit.cloudera.org:8080/#/c/17145/2/be/src/exec/hdfs-table-sink.cc@262 PS2, Line 262: // When an external FE has provided a staging directory we use that directly. > Do we need any validation on the path here to avoid security issues writing For now we are treating the external planner port as a trusted/protected service meant for deployment in scenarios you can lock down access to said port. We should certainly in the (near) future look into doing similar things that communication between impalads do that prevent users from impersonating a coordinator. http://gerrit.cloudera.org:8080/#/c/17145/2/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java File fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java: http://gerrit.cloudera.org:8080/#/c/17145/2/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@90 PS2, Line 90: > I would say "specifies depth" rather than hint as this needs to be enforced Done http://gerrit.cloudera.org:8080/#/c/17145/2/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@243 PS2, Line 243: hdfsTableSink.setSort_columns(sortColumns_); > externalStagingDir_ is not a pointer.. externalStagingDir_ is a String object and if it is not set I do believe it can be null. (Maybe you read it as the int externalStagingPartitionDepth_?) http://gerrit.cloudera.org:8080/#/c/17145/2/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/17145/2/fe/src/main/java/org/apache/impala/service/Frontend.java@1803 PS2, Line 1803: // This is public to allow external frontends to utilize this method to fill in the > Add comment why this is public static. Done -- To view, visit http://gerrit.cloudera.org:8080/17145 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b Gerrit-Change-Number: 17145 Gerrit-PatchSet: 3 Gerrit-Owner: John Sherman <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: John Sherman <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Comment-Date: Mon, 08 Mar 2021 15:11:32 +0000 Gerrit-HasComments: Yes
