Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11358 )
Change subject: IMPALA-7510. Support principals/privileges with LocalCatalog ...................................................................... Patch Set 6: (5 comments) http://gerrit.cloudera.org:8080/#/c/11358/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/11358/3/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@738 PS3, Line 738: I read this as a bug originally until I saw that newCatalogVersion may be updated on L782. Perhaps use two variables for clarity, such as currentCatalogVersion and nextCatalogVersion? http://gerrit.cloudera.org:8080/#/c/11358/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java: http://gerrit.cloudera.org:8080/#/c/11358/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@235 PS6, Line 235: retried via nit: pushed from http://gerrit.cloudera.org:8080/#/c/11358/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@750 PS6, Line 750: sequencer since its specific to auth, pls call this authSequencer or something like this. http://gerrit.cloudera.org:8080/#/c/11358/6/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@862 PS6, Line 862: LOG.error("Error adding privilege: ", e); For the case that we got here via a direct ddl, wouldn't the caller want to know that the 'add' didn't succeed? Separate from this change, but error propagation is not the clearest with this path (same goes for existing ImpaladCatalog). http://gerrit.cloudera.org:8080/#/c/11358/6/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/11358/6/fe/src/main/java/org/apache/impala/catalog/local/LocalCatalog.java@214 PS6, Line 214: // here. is isReady sufficient here? from catalogmetaprovider, its ready when its advanced past the initial version. -- To view, visit http://gerrit.cloudera.org:8080/11358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iccce5aabdb6afe466fdaeae0fb3700c66e658558 Gerrit-Change-Number: 11358 Gerrit-PatchSet: 6 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 05 Sep 2018 19:12:28 +0000 Gerrit-HasComments: Yes
