Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10629 )
Change subject: IMPALA-7137. Support configuring Frontend to use LocalCatalog ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/10629/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10629/2//COMMIT_MSG@21 PS2, Line 21: except when a full catalog update is received. > just to clarify, state that this is the behavior today. Done http://gerrit.cloudera.org:8080/#/c/10629/2/be/src/runtime/exec-env.cc File be/src/runtime/exec-env.cc: http://gerrit.cloudera.org:8080/#/c/10629/2/be/src/runtime/exec-env.cc@87 PS2, Line 87: required > nit: used it's also not required to be started. I'll clarify. http://gerrit.cloudera.org:8080/#/c/10629/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/10629/2/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java@167 PS2, Line 167: name_.toLowerCase() > there have been a number of bugs with inconsistent case handling. just wond in the prior patch in this series I added preconditions to ensure that name_ is always lower case, so I"ll drop this. http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java File fe/src/main/java/org/apache/impala/service/FeCatalogManager.java: http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@30 PS2, Line 30: Managers > nit: Manages Done http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@30 PS2, Line 30: implementations > implementation? I assume its only one at a time. Done http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/FeCatalogManager.java@126 PS2, Line 126: } > nit: add a newline for consistency Done http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/Frontend.java File fe/src/main/java/org/apache/impala/service/Frontend.java: http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/Frontend.java@168 PS2, Line 168: authzChecker_ > where is this set/used? in the constructor, by the policy reader task. http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/Frontend.java@187 PS2, Line 187: /** : * C'tor used by tests to pass in a custom ImpaladCatalog. : */ > stale comment? this looks like the core constructor so should it be public? yea, you're right, this can be private since the above two constructors are the ones meant for external consumption. http://gerrit.cloudera.org:8080/#/c/10629/2/fe/src/main/java/org/apache/impala/service/Frontend.java@197 PS2, Line 197: policyReaderTask.run(); > why is this run here? I would think scheduling on L204 would do this. if ne I agree this is a little goofy, though I was trying to maintain the exact old behavior. In the old code, the constructor created the AuthorizationChecker on like 187 with code that duplicated the body of 'AuthorizationPolicyReader.run'. Instead of duplicating it I figured it would be easier to just call 'run' here, which takes care of setting the authzChecker_ value. The scheduling of the task sets a randomized initial delay to reload, but I think we always need to do one load right at startup to ensure that we have the policy loaded before beginning service, right? I was also confused about why it has the check 'if authzConfig_.isEnabled()' below before scheduling the reloader task, but unconditionally does the initial load. -- To view, visit http://gerrit.cloudera.org:8080/10629 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c9665bd031d23608740b23eef301970af9aa764 Gerrit-Change-Number: 10629 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Tue, 12 Jun 2018 00:25:03 +0000 Gerrit-HasComments: Yes
