Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12221 )
Change subject: IMPALA-5872: Testcase builder for query planner ...................................................................... Patch Set 3: (7 comments) The authorization looks correct, but we need to have more tests. http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java File fe/src/main/java/org/apache/impala/catalog/Catalog.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@66 PS3, Line 66: protected MetaStoreClientPool metaStoreClientPool_ = new MetaStoreClientPool(0, 0); i would still make it final and just set it to null in the Catalog() constructor. http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/Catalog.java@97 PS3, Line 97: , nit: space after comma http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@289 PS3, Line 289: nit: remove extra line http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@341 PS3, Line 341: public void testCopyTestCasePrivileges() throws ImpalaException { We need to add more test cases similar to testSelect() in order to test the actual privileges needed, i.e. VIEW_METADATA. http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@368 PS3, Line 368: nit: fix indentation http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java File fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java: http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java@35 PS3, Line 35: nit: extra space http://gerrit.cloudera.org:8080/#/c/12221/3/fe/src/test/java/org/apache/impala/testutil/EmbeddedMetastoreClientPool.java@42 PS3, Line 42: private Path derbyDataStorePath_; can be final -- To view, visit http://gerrit.cloudera.org:8080/12221 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iec83eeb2dc5136768b70ed581fb8d3ed0335cb52 Gerrit-Change-Number: 12221 Gerrit-PatchSet: 3 Gerrit-Owner: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Balazs Jeszenszky <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Greg Rahn <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Comment-Date: Wed, 30 Jan 2019 17:10:09 +0000 Gerrit-HasComments: Yes
