Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/24852 )
Change subject: IMPALA-15358: Cap Kudu DML writers to partition count ...................................................................... Patch Set 12: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/24852/12//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24852/12//COMMIT_MSG@9 PS12, Line 9: For INSERT/UPSERT into a partitioned Kudu table, the planner inserts a : KUDU-partitioned exchange ahead of the KuduTableSink. At runtime, : KrpcDataStreamSender routes each row to channel (partition_index % : num_channels), where partition_index is the actual Kudu tablet index : (bounded by the table's tablet count). The writer fragment's instance : count, however, was sized like any other fragment (mt_dop / cost-based : scaling) with no awareness of the target table's partitioning. I'm curious where KuduPartitioner is found in this bigger picture. Perhaps, I should have checked out the code myself, but I guess it's simpler to ask this here. Essentially, I'm trying to understand the following: when the number of writer fragment count is equal to the number of Kudu tablets, does each writer fragment has a set of rows that are dedicated to a particular channel/tablet or that's a mix? In other words, on which level does KuduPartitioner is applied here? http://gerrit.cloudera.org:8080/#/c/24852/12/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/24852/12/common/thrift/ImpalaService.thrift@565 PS12, Line 565: inserts nit: doesn't this apply to Kudu's upserts as well? http://gerrit.cloudera.org:8080/#/c/24852/12/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java: http://gerrit.cloudera.org:8080/#/c/24852/12/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@280 PS12, Line 280: maxFsWriters nit: given it's now applicable to Kudu, maybe 'maxWriters' would be a better name for this variable? http://gerrit.cloudera.org:8080/#/c/24852/12/fe/src/main/java/org/apache/impala/planner/KuduTableSink.java File fe/src/main/java/org/apache/impala/planner/KuduTableSink.java: http://gerrit.cloudera.org:8080/#/c/24852/12/fe/src/main/java/org/apache/impala/planner/KuduTableSink.java@114 PS12, Line 114: maxKuduSinks_ > 0 nit: why not to use hasInstanceLimit() here? http://gerrit.cloudera.org:8080/#/c/24852/12/fe/src/main/java/org/apache/impala/planner/KuduTableSink.java@124 PS12, Line 124: maxKuduSinks_ > 0 nit: ditto -- why not to use hasInstanceLimit() here as well? -- To view, visit http://gerrit.cloudera.org:8080/24852 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia3644fb245ceac72e91ed70c8c0392f65df75fde Gerrit-Change-Number: 24852 Gerrit-PatchSet: 12 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: David Rorke <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 02 Oct 2026 17:19:58 +0000 Gerrit-HasComments: Yes
