You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "John Sherman (Code Review)" <ge...@cloudera.org> on 2021/03/01 17:09:07 UTC
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Hello Kurt Deschler,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/17145
to review the following change.
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
IMPALA-10552: External Frontend CTAS support
- Adds the concept of an external staging dir to HdfsTableSink
- This allows an external to specify the destination of the
sink
- When this is set, the external frontend is responsible for
for moving and managing the results
- External frontends may optionally supply a partition
depth which acts as a hint to skip a certain number of
partitions while creating directories for partitions. This
is for when the external frontend has pre-created a
certain number of the directories in staging (usually the
static portion of a partition specification)/
- Modifies delta/base naming to include 0 prefix padding to
match Hive for dynamic partitioning detection
Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 124 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/17145/1
--
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: newchange
Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Gerrit-Change-Number: 17145
Gerrit-PatchSet: 1
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kurt Deschler, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17145
to look at the new patch set (#6).
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
IMPALA-10552: External Frontend CTAS support
- Adds the concept of an external staging dir to HdfsTableSink
- This allows an external to specify the destination of the
sink
- When this is set, the external frontend is responsible for
for moving and managing the results
- Any DDL related operations are assumed to be handled by
the external frontend
- External frontends may optionally supply a partition
depth which acts as a hint to skip a certain number of
partitions while creating directories for partitions. This
is for when the external frontend has pre-created a
certain number of the directories in staging (usually the
static portion of a partition specification)/
- Modifies delta/base naming to include 0 prefix padding to
match Hive for dynamic partitioning detection
- External frontends are responsible for doing authorization
checks against the staging directory and it is assumed the
external frontend service is not exposed directly to users.
Co-authored-by: Kurt Deschler <kd...@cloudera.com>
Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 123 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/17145/6
--
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: newpatchset
Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Gerrit-Change-Number: 17145
Gerrit-PatchSet: 6
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 3: Verified+1
--
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 <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 21:21:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
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 <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 15:11:32 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 1:
(1 comment)
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
--
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: 1
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 17:12:21 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8362/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 6
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sun, 14 Mar 2021 18:57:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17145/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17145/4//COMMIT_MSG@12
PS4, Line 12: - When this is set, the external frontend is responsible for
: for moving and managing the results
> This is correct. I'll update the commit message to make that clearer.
Done
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc@127
PS4, Line 127: staging_dir_ = Substitute("$0/_impala_insert_staging/$1", table_desc_->hdfs_base_dir(),
: PrintId(state->query_id(), "_"));
:
: // Prepare partition key exprs and gather dynamic partition key exprs.
: for (size_t i = 0; i < partition_key_expr_evals_.size(); ++i) {
: // Remember non-constant partition key exprs for building hash table of Hdfs files.
:
> I can certainly change everything to be external output rather than externa
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: 7
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sun, 14 Mar 2021 18:40:41 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 8:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6971/ DRY_RUN=true
--
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: 8
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Mar 2021 21:54:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 2:
gerrit-verify-dryrun-external
--
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: 2
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Sun, 07 Mar 2021 16:38:59 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 4:
(3 comments)
Thanks Joe for the feedback
http://gerrit.cloudera.org:8080/#/c/17145/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17145/4//COMMIT_MSG@12
PS4, Line 12: - When this is set, the external frontend is responsible for
: for moving and managing the results
> I assume the external frontend is also handling any metadata operations as
This is correct. I'll update the commit message to make that clearer.
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc@127
PS4, Line 127: if (HasExternalStagingDir()) {
: // When an external FE has provided a staging path, we use it directly.
: staging_dir_ = external_staging_dir_;
: } else {
: staging_dir_ = Substitute("$0/_impala_insert_staging/$1",
: table_desc_->hdfs_base_dir(), PrintId(state->query_id(), "_"));
: }
> The external staging directory is a staging directory from the point of vie
I can certainly change everything to be external output rather than external staging.
I'll verify why I'm modifying staging_dir_ here - reading the code I assumed I initially did this so BuildHdfsFileNames would use it correctly to build tmp_hdfs_* variables but indeed I don't think these paths would utilize them.
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc@279
PS4, Line 279: // The 0 padding on base and delta is to match the behavior of Hive since various
: // systems will expect a certain format for dynamic partition creation. Additionally
: // include an 0 statement id for delta directory so various Hive AcidUtils detect
: // the directory (such as AcidUtils.baseOrDeltaSubdir()). Multiple statements in a
: // single transaction is not supported.
: if (overwrite_) {
: output_partition->final_hdfs_file_name_prefix += StringPrintf("/base_%07ld/",
: write_id_);
: } else {
: output_partition->final_hdfs_file_name_prefix += StringPrintf(
: "/delta_%07ld_%07ld_0000/", write_id_, write_id_);
: }
> Should this be triggered by its own variable independently from the externa
It is a bit orthogonal. I think ideally Impala/Hive and external frontend implementations would standardize on something consistent. I took the paranoid approach of only applying this to external frontends as I do not want to cause any inadvertent regressions. I'm not sure it is currently worth it to plumb through a flag indicating which format is expected. If another implementation of a frontend shows up,I think it might be worth it. Feel free to push back if you feel strongly.
--
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: 4
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 22:15:51 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kurt Deschler, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17145
to look at the new patch set (#3).
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
IMPALA-10552: External Frontend CTAS support
- Adds the concept of an external staging dir to HdfsTableSink
- This allows an external to specify the destination of the
sink
- When this is set, the external frontend is responsible for
for moving and managing the results
- External frontends may optionally supply a partition
depth which acts as a hint to skip a certain number of
partitions while creating directories for partitions. This
is for when the external frontend has pre-created a
certain number of the directories in staging (usually the
static portion of a partition specification)/
- Modifies delta/base naming to include 0 prefix padding to
match Hive for dynamic partitioning detection
Co-authored-by: Kurt Deschler <kd...@cloudera.com>
Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 129 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/17145/3
--
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: newpatchset
Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Gerrit-Change-Number: 17145
Gerrit-PatchSet: 3
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 1:
(4 comments)
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", table_desc_->hdfs_base_dir(),
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc@232
PS1, Line 232: void HdfsTableSink::BuildHdfsFileNames(const HdfsPartitionDescriptor& partition_descriptor,
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc@506
PS1, Line 506: bool external_part = HasExternalStagingDir() && j >= external_staging_partition_depth_;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/17145/1/be/src/exec/hdfs-table-sink.cc@549
PS1, Line 549: BuildHdfsFileNames(partition_descriptor, output_partition, external_partition_name_ss.str());
line too long (95 > 90)
--
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: 1
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 17:10:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kurt Deschler, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17145
to look at the new patch set (#7).
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
IMPALA-10553: External Frontend CTAS support
- Adds the concept of an external staging dir to HdfsTableSink
- This allows an external to specify the destination of the
sink
- When this is set, the external frontend is responsible for
for moving and managing the results
- Any DDL related operations are assumed to be handled by
the external frontend
- External frontends may optionally supply a partition
depth which acts as a hint to skip a certain number of
partitions while creating directories for partitions. This
is for when the external frontend has pre-created a
certain number of the directories in staging (usually the
static portion of a partition specification)/
- Modifies delta/base naming to include 0 prefix padding to
match Hive for dynamic partitioning detection
- External frontends are responsible for doing authorization
checks against the staging directory and it is assumed the
external frontend service is not exposed directly to users.
Co-authored-by: Kurt Deschler <kd...@cloudera.com>
Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 123 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/17145/7
--
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: newpatchset
Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Gerrit-Change-Number: 17145
Gerrit-PatchSet: 7
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 8:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8372/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 8
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Mar 2021 22:09:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 8: Verified+1
--
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: 8
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 16 Mar 2021 03:34:40 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17145/7/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:
http://gerrit.cloudera.org:8080/#/c/17145/7/be/src/exec/hdfs-table-sink.cc@257
PS7, Line 257: // When an external FE has provided a staging directory we use that directly.
: // We are trusting that the external frontend implementation has done appropriate
: // authorization checks on the external output directory.
> Nit: Update comment to call it an external output directory rather than a s
Done
http://gerrit.cloudera.org:8080/#/c/17145/7/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/7/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@250
PS7, Line 250: if (writeId_ != -1) hdfsTableSink.setWrite_id(writeId_);
: TTableSink tTableSink = new TTableSink(DescriptorTable.TABLE_SINK_ID,
:
> Nit: If the line hasn't changed, avoid style-only changes.
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: 8
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Mar 2021 21:50:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8309/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 4
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 17:26:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17145/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17145/4//COMMIT_MSG@12
PS4, Line 12: - When this is set, the external frontend is responsible for
: for moving and managing the results
I assume the external frontend is also handling any metadata operations as well. Is that right?
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc@127
PS4, Line 127: if (HasExternalStagingDir()) {
: // When an external FE has provided a staging path, we use it directly.
: staging_dir_ = external_staging_dir_;
: } else {
: staging_dir_ = Substitute("$0/_impala_insert_staging/$1",
: table_desc_->hdfs_base_dir(), PrintId(state->query_id(), "_"));
: }
The external staging directory is a staging directory from the point of view of an external frontend, but in this file, it doesn't use the staging codepath. It uses the direct writing codepath with an alternate location (i.e. ShouldSkipStaging() returns true and it customizes final_hdfs_file_name_prefix).
So, I think there is no need to set staging_dir_ to something different. That doesn't interact with the external staging directory codepath.
Separately, what do you think about calling this something other than a staging directory? Custom output location? External output directory?
http://gerrit.cloudera.org:8080/#/c/17145/4/be/src/exec/hdfs-table-sink.cc@279
PS4, Line 279: // The 0 padding on base and delta is to match the behavior of Hive since various
: // systems will expect a certain format for dynamic partition creation. Additionally
: // include an 0 statement id for delta directory so various Hive AcidUtils detect
: // the directory (such as AcidUtils.baseOrDeltaSubdir()). Multiple statements in a
: // single transaction is not supported.
: if (overwrite_) {
: output_partition->final_hdfs_file_name_prefix += StringPrintf("/base_%07ld/",
: write_id_);
: } else {
: output_partition->final_hdfs_file_name_prefix += StringPrintf(
: "/delta_%07ld_%07ld_0000/", write_id_, write_id_);
: }
Should this be triggered by its own variable independently from the external staging directory? It seems orthogonal to where the output goes.
--
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: 4
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 20:52:53 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 7: Code-Review+2
(2 comments)
This looks good to me, only a couple style-only nits. Thanks for changing the naming for this.
http://gerrit.cloudera.org:8080/#/c/17145/7/be/src/exec/hdfs-table-sink.cc
File be/src/exec/hdfs-table-sink.cc:
http://gerrit.cloudera.org:8080/#/c/17145/7/be/src/exec/hdfs-table-sink.cc@257
PS7, Line 257: // When an external FE has provided a staging directory we use that directly.
: // We are trusting that the external frontend implementation has done appropriate
: // authorization checks on the external staging directory.
Nit: Update comment to call it an external output directory rather than a staging directory.
http://gerrit.cloudera.org:8080/#/c/17145/7/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/7/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@250
PS7, Line 250: if (writeId_ != -1) {
: hdfsTableSink.setWrite_id(writeId_);
: }
Nit: If the line hasn't changed, avoid style-only changes.
--
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: 7
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Mar 2021 19:56:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 2:
(2 comments)
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:
> Done
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: output_partition->final_hdfs_file_name_prefix = Substitute("$0/$1/",
> Sure, I can add a comment here. I didn't initially because if I added a com
I added a comment here and to the commit message for this change set.
--
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: 2
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 17:07:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6939/ DRY_RUN=true
--
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 <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 15:39:01 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
Patch Set 8: Code-Review+2
Looks good
--
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: 8
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 15 Mar 2021 21:51:34 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 2:
(5 comments)
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:
Extra line
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: output_partition->final_hdfs_file_name_prefix = Substitute("$0/$1/",
Do we need any validation on the path here to avoid security issues writing to an arbitrary HDFS directory? Mainly concerned because the path prefix is supplied externally.
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: // This acts as a hint to the sink on how deep into the partition specification in
I would say "specifies depth" rather than hint as this needs to be enforced.
http://gerrit.cloudera.org:8080/#/c/17145/2/fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java@243
PS2, Line 243: if (externalStagingDir_ != null) {
externalStagingDir_ is not a pointer..
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: public static void addFinalizationParamsForInsert(
Add comment why this is public static.
--
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: 2
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 13:07:23 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 2:
(3 comments)
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: output_partition->final_hdfs_file_name_prefix = Substitute("$0/$1/",
> Please add a comment to this effect. We may want to at least validate the p
Sure, I can add a comment here. I didn't initially because if I added a comment everywhere where this is possible it'd be a lot of comments. The commit message for the external planner port makes it a bit clearer too
"This allows different security policy to be applied to
each type of connection. The external_fe_port should be considered
a privileged service and should only be exposed to an external
frontend that does user authentication and does authorization
checks on generated plans"
In any case, I do not mind being paranoid via comments in such a case.
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@243
PS2, Line 243: if (externalStagingDir_ != null) {
> Actually was thinking C++. This is Java an you are correct.
Done
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: public static void addFinalizationParamsForInsert(
> No comment added?
I think you are looking at patch set 2 and not 3
--
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: 2
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 16:51:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kurt Deschler, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17145
to look at the new patch set (#4).
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
IMPALA-10552: External Frontend CTAS support
- Adds the concept of an external staging dir to HdfsTableSink
- This allows an external to specify the destination of the
sink
- When this is set, the external frontend is responsible for
for moving and managing the results
- External frontends may optionally supply a partition
depth which acts as a hint to skip a certain number of
partitions while creating directories for partitions. This
is for when the external frontend has pre-created a
certain number of the directories in staging (usually the
static portion of a partition specification)/
- Modifies delta/base naming to include 0 prefix padding to
match Hive for dynamic partitioning detection
- External frontends are responsible for doing authorization
checks against the staging directory and it is assumed the
external frontend service is not exposed directly to users.
Co-authored-by: Kurt Deschler <kd...@cloudera.com>
Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 131 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/17145/4
--
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: newpatchset
Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Gerrit-Change-Number: 17145
Gerrit-PatchSet: 4
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8268/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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: 1
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 18:53:53 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
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.
> Sure, I can add a comment here. I didn't initially because if I added a com
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: 4
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 19:38:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 2:
(3 comments)
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: output_partition->final_hdfs_file_name_prefix = Substitute("$0/$1/",
> For now we are treating the external planner port as a trusted/protected se
Please add a comment to this effect. We may want to at least validate the prefix and depth arguments are consistent with what is expected here.
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@243
PS2, Line 243: if (externalStagingDir_ != null) {
> externalStagingDir_ is a String object and if it is not set I do believe it
Actually was thinking C++. This is Java an you are correct.
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: public static void addFinalizationParamsForInsert(
> Done
No comment added?
--
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: 2
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 15:56:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
John Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 1:
(1 comment)
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:
delete
--
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: 1
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 17:09:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "John Sherman (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Kurt Deschler, Joe McDonnell, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17145
to look at the new patch set (#8).
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
IMPALA-10553: External Frontend CTAS support
- Adds the concept of an external staging dir to HdfsTableSink
- This allows an external to specify the destination of the
sink
- When this is set, the external frontend is responsible for
for moving and managing the results
- Any DDL related operations are assumed to be handled by
the external frontend
- External frontends may optionally supply a partition
depth which acts as a hint to skip a certain number of
partitions while creating directories for partitions. This
is for when the external frontend has pre-created a
certain number of the directories in staging (usually the
static portion of a partition specification)/
- Modifies delta/base naming to include 0 prefix padding to
match Hive for dynamic partitioning detection
- External frontends are responsible for doing authorization
checks against the staging directory and it is assumed the
external frontend service is not exposed directly to users.
Co-authored-by: Kurt Deschler <kd...@cloudera.com>
Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 120 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/45/17145/8
--
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: newpatchset
Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Gerrit-Change-Number: 17145
Gerrit-PatchSet: 8
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10553: External Frontend CTAS support
Posted by "Joe McDonnell (Code Review)" <ge...@cloudera.org>.
Joe McDonnell has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10553: External Frontend CTAS support
......................................................................
IMPALA-10553: External Frontend CTAS support
- Adds the concept of an external staging dir to HdfsTableSink
- This allows an external to specify the destination of the
sink
- When this is set, the external frontend is responsible for
for moving and managing the results
- Any DDL related operations are assumed to be handled by
the external frontend
- External frontends may optionally supply a partition
depth which acts as a hint to skip a certain number of
partitions while creating directories for partitions. This
is for when the external frontend has pre-created a
certain number of the directories in staging (usually the
static portion of a partition specification)/
- Modifies delta/base naming to include 0 prefix padding to
match Hive for dynamic partitioning detection
- External frontends are responsible for doing authorization
checks against the staging directory and it is assumed the
external frontend service is not exposed directly to users.
Co-authored-by: Kurt Deschler <kd...@cloudera.com>
Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Reviewed-by: Kurt Deschler <kd...@cloudera.com>
Reviewed-on: http://gerrit.cloudera.org:8080/17145
Reviewed-by: Joe McDonnell <jo...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M common/thrift/DataSinks.thrift
M fe/src/main/java/org/apache/impala/planner/HdfsTableSink.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
5 files changed, 120 insertions(+), 17 deletions(-)
Approvals:
Joe McDonnell: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: Iae0ea4a832d8281c563427d0d7da1623bfce437b
Gerrit-Change-Number: 17145
Gerrit-PatchSet: 9
Gerrit-Owner: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10552: External Frontend CTAS support
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17145 )
Change subject: IMPALA-10552: External Frontend CTAS support
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8306/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
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 <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 15:27:32 +0000
Gerrit-HasComments: No