Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/10627 )
Change subject: IMPALA-7135. Skeleton implementation of LocalCatalog ...................................................................... Patch Set 2: (13 comments) http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java@49 PS2, Line 49: : new MetaStor nit: single line http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java File fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java: http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@53 PS2, Line 53: which does not interact with catalogd Although it could, if a hypothetical CatalogdMetaProvider is implemented (along with needed catalogd changes). The design does not preclude it, afaict? Lets drop this last part, since in time, it will become stale and/or the context may be lost. http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@72 PS2, Line 72: metaProvider precondition to assert non-null. http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@76 PS2, Line 76: matcher where's this used? http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@88 PS2, Line 88: list nit: names (seems more intuitive) http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java File fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java: http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@44 PS2, Line 44: name_ lowercase? http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@53 PS2, Line 53: catalog, String dbName precondition for non-null http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@70 PS2, Line 70: c nit: C http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@71 PS2, Line 71: e); nit: pull up to prev. line http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@85 PS2, Line 85: assert tb use precondition, and check non-null. http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@122 PS2, Line 122: c nit: C http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java File fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java: http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/LocalTable.java@42 PS2, Line 42: name_ lowercase? http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java: http://gerrit.cloudera.org:8080/#/c/10627/2/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java@33 PS2, Line 33: * or may include caching, etc. so far, this covers a portion of hms. pls add a todo for covering other hms apis, fs, and sentry. -- To view, visit http://gerrit.cloudera.org:8080/10627 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab653371188b21c72f50ee1ec4e94950aa6fb9ee Gerrit-Change-Number: 10627 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Mon, 11 Jun 2018 05:33:08 +0000 Gerrit-HasComments: Yes
