Michael Smith 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 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/24852/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24852/6//COMMIT_MSG@9 PS6, Line 9: UPSERT into a partit > Can we tests for these as well? Done http://gerrit.cloudera.org:8080/#/c/24852/6//COMMIT_MSG@25 PS6, Line 25: > Could you please add e2e tests that check #Inst for KUDU WRITER from the ex Done http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/analysis/KuduPartitionParam.java File fe/src/main/java/org/apache/impala/analysis/KuduPartitionParam.java: http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/analysis/KuduPartitionParam.java@226 PS6, Line 226: .addAll(colNames); > Unused? Done http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java File fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java: http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@166 PS6, Line 166: uilder(kuduTable) > Deprecated API, could use KuduPartitioner.numPartitions() Done http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@169 PS6, Line 169: Exception > Do we hit this for CTAS statements? Yes, I've fixed it. http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/catalog/FeKuduTable.java@170 PS6, Line 170: throw new ImpalaRuntimeException("Error accessing Kudu for tablet locations.", : e); > Maybe we could just derive the result from partition params? Or return -1? Changed it so we don't fail the query if we can't reach Kudu, although it will likely fail soon after. http://gerrit.cloudera.org:8080/#/c/24852/6/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/6/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@354 PS6, Line 354: om its part > Should be IMPALA-15358? Yeah, dropped the ticket mention. IMPALA-5254 isn't really related to the limit on partition targets. http://gerrit.cloudera.org:8080/#/c/24852/6/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@359 PS6, Line 359: ? FeKuduTable.Utils.estimateNumPartitions(kuduTable.getPartitionBy()) : : FeKuduTable.Utils.getNumPartitions(kuduTable); : } catch (ImpalaException e) { > Can we document that MAX_FS_WRITERS now caps Kudu writers as well? I've updated ImpalaService.thrift's doc. MAX_FS_WRITERS isn't yet documented on our site: https://issues.apache.org/jira/browse/IMPALA-13839. http://gerrit.cloudera.org:8080/#/c/24852/6/testdata/workloads/functional-planner/queries/PlannerTest/kudu-insert-writer-limit.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu-insert-writer-limit.test: http://gerrit.cloudera.org:8080/#/c/24852/6/testdata/workloads/functional-planner/queries/PlannerTest/kudu-insert-writer-limit.test@1 PS6, Line 1: capped at the : # table's tablet count (3), even > This isn't true anymore. Done -- 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: 8 Gerrit-Owner: Michael Smith <[email protected]> Gerrit-Reviewer: Aleksandr Efimov <[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: Tue, 29 Sep 2026 22:24:38 +0000 Gerrit-HasComments: Yes
