Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/10584 )
Change subject: IMPALA-7077. Add a configuration for the max number of partitions to load ...................................................................... Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/10584/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10584/1//COMMIT_MSG@19 PS1, Line 19: partitions will be created You mean those are created on the filesystem but not in HMS or something else? http://gerrit.cloudera.org:8080/#/c/10584/1//COMMIT_MSG@21 PS1, Line 21: worth the complexity to try to : somehow limit the addition of partitions during a DML I guess this is a simple check in CatalogOpEx#updateCatalog() unless I'm missing something. http://gerrit.cloudera.org:8080/#/c/10584/1/be/src/util/backend-gflag-util.cc File be/src/util/backend-gflag-util.cc: http://gerrit.cloudera.org:8080/#/c/10584/1/be/src/util/backend-gflag-util.cc@72 PS1, Line 72: nit: remove http://gerrit.cloudera.org:8080/#/c/10584/1/common/thrift/CatalogService.thrift File common/thrift/CatalogService.thrift: http://gerrit.cloudera.org:8080/#/c/10584/1/common/thrift/CatalogService.thrift@68 PS1, Line 68: 7: optional list<string> warning_messages Where is this used? Don't see it's references in the patch. http://gerrit.cloudera.org:8080/#/c/10584/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/10584/1/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2067 PS1, Line 2067: if (curPartitionCount + allHmsPartitionsToAdd.size() > HdfsTable.MAX_PARTITIONS_PER_TABLE) { nit: line overflow (here and below) http://gerrit.cloudera.org:8080/#/c/10584/1/testdata/workloads/functional-query/queries/QueryTest/max-partitions-to-load.test File testdata/workloads/functional-query/queries/QueryTest/max-partitions-to-load.test: http://gerrit.cloudera.org:8080/#/c/10584/1/testdata/workloads/functional-query/queries/QueryTest/max-partitions-to-load.test@52 PS1, Line 52: insert into $DATABASE.t1(y) values (21) Could you add a test wherein partitions are added from Hive and user does a REFRESH and that hits the partition limit? http://gerrit.cloudera.org:8080/#/c/10584/1/testdata/workloads/functional-query/queries/QueryTest/max-partitions-to-load.test@70 PS1, Line 70: I think the patch is missing the guard rails in "ALTER TABLE <foo> RECOVER PARTITIONS" case. Could you fix that and add a test? -- To view, visit http://gerrit.cloudera.org:8080/10584 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife99a97a891ed14675303ea472abb2932a72cb51 Gerrit-Change-Number: 10584 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Mon, 04 Jun 2018 23:34:33 +0000 Gerrit-HasComments: Yes
