You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2019/03/13 22:15:01 UTC
[Impala-ASF-CR] IMPALA-8293 (WIP): Support for Ranger cache invalidation
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12748
Change subject: IMPALA-8293 (WIP): Support for Ranger cache invalidation
......................................................................
IMPALA-8293 (WIP): Support for Ranger cache invalidation
This patch addes support for Ranger cache invalidation via INVALIDATE
METADATA and REFRESH AUTHORIZATION. This patch uses a hacked way of
invalidating Ranger caches and it will be fixed as soon the API in
RANGER-2349 is available.
Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
---
M common/thrift/CatalogObjects.thrift
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationManager.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
A fe/src/main/java/org/apache/impala/catalog/AuthzCache.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeCatalogManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
28 files changed, 535 insertions(+), 115 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/12748/1
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2569/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 01:51:02 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8293 (WIP): Support for Ranger cache invalidation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293 (WIP): Support for Ranger cache invalidation
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2428/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Mar 2019 22:44:11 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 6:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/12748/6/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:
http://gerrit.cloudera.org:8080/#/c/12748/6/common/thrift/CatalogObjects.thrift@590
PS6, Line 590: THrift
typo: Thrift
Also I think it's worth noting that this is just used for invalidation, and doesn't actually carry cached authz data?
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@121
PS6, Line 121: * Performs a cache refresh.
> Can you add more information? How is this cache related to the Authorizatio
yea I'm slightly confused what the difference is between calling AuthorizationChecker.refresh() and AuthorizationRefresher.refresh(). I guess maybe the AuthorizationRefresher.refresh() is called on the catalogd and AuthorizationChecker.refresh() is called on the impalad in response to the catalog object showing up indicating a refresh is needed or something?
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java@27
PS6, Line 27: * This class keeps track of authorization catalog objects added/removed.
can you expand this a little bit? Are authorization catalog objects restricted to a certain subset of types of catalog objects?
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java@41
PS6, Line 41: removed_ = removed;
nit: can combine the checkNotNull() into these assignments, since it returns its argument.
Also consider wrapping in ImmutableList.copyOf() to be sure that it doesn't change underneath us
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@75
PS6, Line 75: AuthorizationManager newAuthorizationManager(CatalogServiceCatalog catalog,
: AuthorizationRefresher authzRefresher);
starting to get a bit confused about the relationships of these classes here. Specifically, it seems strange that this method takes an AuthorizationRefresher, but the factory itself also provides a method to create an AuthorizationRefresher. Perhaps an interface-level javadoc explaining what the different "component interfaces" are used for, what their lifecycle is, etc, would be useful?
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java@29
PS6, Line 29: object
object(s)
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java@30
PS6, Line 30: rest
> typo: reset
Can you use @param javadoc format?
Also, I think it's clearer to document the effect of 'resetVersions', not just the context in which it's set to true. I'm still not sure what it means to set it to true by reading this.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@165
PS6, Line 165: NoneAuthorizationRefresher
nit: I think "NoopAuthorizationRefresher' is clearer that it is a refresher (ie it's not none) but it does nothing when called.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@64
PS6, Line 64: synchronized
> Can we limit the scope of the synchronization to just the `authorize` calls
yea I'm guessing that this can call out to a remote service in some cases, so synchronizing it may be unnecessarily expensive for query concurrency.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@160
PS6, Line 160: plugin_.init();
should we consider an AtomicReference<Plugin> and create a new plugin instead? then we don't need to synchronize
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java@36
PS6, Line 36: public RangerAuthorizationRefresher(CatalogServiceCatalog catalog) {
I wonder whether passing the catalog as an argument to refresh() would be cleaner.. it seems like in both cases (Ranger and Sentry), refresh() is called from the context of a catalog, and is expected to mutate catalog by adding/removing various catalog objects. Taking the catalog reference in the constructor the fact that the 'refresh()' method mutates catalog, which I think is somewhat confusing.
If we moved the 'catalog' argument to refresh(), then we can also document in the AuthorizationRefresher interface what the locking constraints are. For example, it seems like it would be unsafe to call refresh() from a context in which the catalog write lock is held.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java@45
PS6, Line 45: authzDelta.getAdded().add(authzCache.toTCatalogObject());
> nit: Add some method to `AuthorizationDelta` for adding elements to the `pr
yea, agreed it would be better to just do something like new AuthorizationDelta(ImmutableList.of(authzCache.toCatalogObject()), Collections.emptyList())
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@65
PS6, Line 65: Preconditions.checkNotNull(authzRefresher);
nit: can do these inline in the assignments
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@133
PS6, Line 133: authzRefresher_.refresh(false);
I'm surprised that you don't need to return the CatalogDelta from this method back to the caller here, so it knows which catalog updates resulted from the DDL operation
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java@123
PS6, Line 123: public void refresh() {}
worth a comment here explaining why this is a no-op
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java@82
PS6, Line 82: return authzRefresher_.getSentryProxy().isSentryAdmin(user);
seems surprising that the sentry proxy is owned by the refresher instead of this class
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
File fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java@114
PS6, Line 114: kerberosPrincipal
this is no longer the kerberos principal, since kerberos might be disabled
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java@120
PS6, Line 120: processUser_ = kerberosPrincipal;
checkNotNull()
http://gerrit.cloudera.org:8080/#/c/12748/6/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/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2297
PS6, Line 2297: addAuthzCache
> I think this method name is slightly misleading. It only adds a new cache i
Yea I'm finding this logic somewhat hard to follow here as well. Maybe we can discuss this design a bit? In general it seems like there's an effort to put this in some kind of generic abstraction of 'AuthzCache' objects, but the AuthzCache object itself is just a version number tracker and there's only one, and it's used only by ranger? Maybe a short one-page design doc would be useful.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@314
PS6, Line 314: with authorization enabled, but does
: * not do the actual authorization.
: *
: * @param authorized the result of the authorization.
can you clarify this javadoc a bit to say that the authorization factory returns a constant result? eg the param description should say "the result of all authorizations performed by this instance" or something?
Perhaps this anonymous class is getting large enough it's worth breaking out a static inner class like 'ConstantAuthorizationFactory' or 'DummyAuthorizationFactory' something?
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 20:51:16 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8293 (WIP): Support for Ranger cache invalidation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293 (WIP): Support for Ranger cache invalidation
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12748/1/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/12748/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@47
PS1, Line 47: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12748/1/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@55
PS1, Line 55: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/1/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
File fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java:
http://gerrit.cloudera.org:8080/#/c/12748/1/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java@21
PS1, Line 21: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Mar 2019 22:15:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Austin Nobis (Code Review)" <ge...@cloudera.org>.
Austin Nobis has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 6:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java@121
PS6, Line 121: * Performs a cache refresh.
Can you add more information? How is this cache related to the AuthorizationChecker interface.?
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java@44
PS6, Line 44: public List<TCatalogObject> getAdded() { return added_; }
: public List<TCatalogObject> getRemoved() { return removed_; }
nit: Consider wrapping these in Collections.unmodifiableList().
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java@79
PS6, Line 79: of {@link AuthorizationRefresher}
nit: of *an* {@link AuthorizationRefresher}
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java@30
PS6, Line 30: rest
typo: reset
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java@64
PS6, Line 64: synchronized
Can we limit the scope of the synchronization to just the `authorize` calls? I'm guessing we need to synchronize this in the event that the `refresh` is called and we want to wait for it to complete?
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java@45
PS6, Line 45: authzDelta.getAdded().add(authzCache.toTCatalogObject());
nit: Add some method to `AuthorizationDelta` for adding elements to the `private final List<>` as opposed to exposing the list and mutating its elements.
http://gerrit.cloudera.org:8080/#/c/12748/6/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/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2293
PS6, Line 2293: ;
nit: unnecessary semicolon
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2297
PS6, Line 2297: addAuthzCache
I think this method name is slightly misleading. It only adds a new cache if the current authzCache is null. It also ends up looking weird from the functions that call this method.
i.e. https://gerrit.cloudera.org/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java#44
In this case, the RangerAuthorizationRefresher looks like it is adding a new authzCache everytime refresh is called and returning the created object. In reality it is just an increment to the catalog version while returning the mutated authzCache that already existed.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java
File fe/src/main/java/org/apache/impala/service/JniCatalog.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/service/JniCatalog.java@140
PS6, Line 140: User kerberosPrincipal = Strings.isNullOrEmpty(cfg.principal) ?
: new User(System.getProperty("user.name")) : new User(cfg.principal);
Is it safe to use the System.getProperty('user.name") here if the principal is not passed from the backend? I'm not sure if something malicious is possible here by potentially passing -Duser.name=* during start-up. It seems like the User is only used by Sentry and not Ranger, but it still seems like a potential vulnerability.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 19:16:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 6:
I'm abandoning this CR because it contains refactor + new feature, which makes the review difficult. I'll split this work into 2 CRs instead. Some of the comments will be addressed in the new CRs.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 18 Apr 2019 18:00:06 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 5:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12748/5/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/12748/5/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@49
PS5, Line 49: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/5/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/12748/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@38
PS5, Line 38: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/5/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12748/5/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@56
PS5, Line 56: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/12748/5/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@43
PS5, Line 43: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/5/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
File fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java:
http://gerrit.cloudera.org:8080/#/c/12748/5/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java@21
PS5, Line 21: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 01:09:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
IMPALA-8293: Support for Ranger cache invalidation
This patch adds support for Ranger cache invalidation via INVALIDATE
METADATA and REFRESH AUTHORIZATION. This patch introduces a new catalog
object type called AUTHZ_CACHE to allow Catalogd broadcasting messages
to Impalads to update their local Ranger caches. For beter user
experience, every GRANT/REVOKE statement will cause Catalogd to
send AUTHZ_CACHE catalog object to Impalads. This patch uses a hacked
way of invalidating Ranger caches and it will be updated as soon the API
in RANGER-2349 is available in the Ranger minicluster.
This patch further decouples SentryProxy out of CatalogServiceCatalog
and introduces AuthorizationRefresher that can be optionally use as a
mechanism to update authorization cache.
Testing:
- Added new E2E authorization tests
- Ran all FE tests
- Ran all E2E authorization tests
Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
---
M be/src/catalog/catalog-util.cc
M common/thrift/CatalogObjects.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
A fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
A fe/src/main/java/org/apache/impala/catalog/AuthzCache.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeCatalogManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/authorization/sentry/SentryProxyTest.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
M tests/authorization/test_ranger.py
36 files changed, 802 insertions(+), 185 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/12748/5
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 5:
Todd/Bharath, this is ready for review.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 01:09:23 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8293 (WIP): Support for Ranger cache invalidation
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293 (WIP): Support for Ranger cache invalidation
......................................................................
Patch Set 1:
A sneak peak to force Ranger cache invalidation. REFRESH AUTHORIZATION works but INVALIDATE METADATA is still broken. Need figure out what to do with local catalog.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 1
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Wed, 13 Mar 2019 22:16:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/2568/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 01:54:19 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 6:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12748/6/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/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@49
PS6, Line 49: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/6/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/12748/6/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@38
PS6, Line 38: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
File fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java@56
PS6, Line 56: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@43
PS6, Line 43: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
File fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java@21
PS6, Line 21: import org.apache.impala.authorization.NoneAuthorizationFactory.NoneAuthorizationRefresher;
line too long (91 > 90)
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 01:17:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
IMPALA-8293: Support for Ranger cache invalidation
This patch adds support for Ranger cache invalidation via INVALIDATE
METADATA and REFRESH AUTHORIZATION. This patch introduces a new catalog
object type called AUTHZ_CACHE to allow Catalogd broadcasting messages
to Impalads to update their local Ranger caches. For beter user
experience, every GRANT/REVOKE statement will cause Catalogd to
send AUTHZ_CACHE catalog object to Impalads. This patch uses a hacked
way of invalidating Ranger caches and it will be updated as soon the API
in RANGER-2349 is available in the Ranger minicluster.
This patch further decouples SentryProxy out of CatalogServiceCatalog
and introduces AuthorizationRefresher that can be optionally use as a
mechanism to update authorization cache.
Testing:
- Added new E2E authorization tests
- Ran all FE tests
- Ran all E2E authorization tests
Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
---
M be/src/catalog/catalog-util.cc
M common/thrift/CatalogObjects.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/authorization/AuthorizationChecker.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationDelta.java
M fe/src/main/java/org/apache/impala/authorization/AuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
A fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationChecker.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationFactory.java
A fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationRefresher.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryCatalogdAuthorizationManager.java
M fe/src/main/java/org/apache/impala/authorization/sentry/SentryProxy.java
A fe/src/main/java/org/apache/impala/catalog/AuthzCache.java
M fe/src/main/java/org/apache/impala/catalog/Catalog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeCatalogManager.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/authorization/sentry/SentryProxyTest.java
M fe/src/test/java/org/apache/impala/catalog/events/MetastoreEventsProcessorTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/testutil/CatalogServiceTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
M fe/src/test/java/org/apache/impala/testutil/PlannerTestCaseLoader.java
M tests/authorization/test_ranger.py
36 files changed, 801 insertions(+), 184 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/48/12748/6
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Bharath Vissapragada (Code Review)" <ge...@cloudera.org>.
Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 6:
(6 comments)
Haven't looked into it very deeply but like other reviewers suggested, we should probably rethink the class design first before we go into the specifics. I made a suggestion, let me know if that doesn't work for some reason.
http://gerrit.cloudera.org:8080/#/c/12748/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12748/6//COMMIT_MSG@12
PS6, Line 12: beter
typo
http://gerrit.cloudera.org:8080/#/c/12748/6/common/thrift/CatalogObjects.thrift
File common/thrift/CatalogObjects.thrift:
http://gerrit.cloudera.org:8080/#/c/12748/6/common/thrift/CatalogObjects.thrift@590
PS6, Line 590: THrift
> typo: Thrift
I think we should rename it to reflect the intent. Like Todd said, it feels like cache and not an invalidation marker.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java
File fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/AuthorizationRefresher.java@27
PS6, Line 27: public interface AuthorizationRefresher {
High level comment.
I think the class design is confusing. Does it make sense to club this with the AuthzMgr? I'd assume that a given AuthzMgr [Sentry|Ranger][Catalogd|Impalad]Mgr should be able to have overrides for refresh(). That way we have a single AuthzMgr() that all the callers can talk to? Does that not work for some reason?
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@133
PS6, Line 133: authzRefresher_.refresh(false);
> I'm surprised that you don't need to return the CatalogDelta from this meth
Should this also be set in the response object too so that the DDL affects are applied on the coordinators right away?
(Probably Todd meant the same thing and if so, I have the same question)
http://gerrit.cloudera.org:8080/#/c/12748/6/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/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@321
PS6, Line 321: public void setAuthzRefresher(AuthorizationRefresher authzRefresher) {
: Preconditions.checkNotNull(authzRefresher);
: authzRefresher_ = authzRefresher;
: }
Like I mentioned elsewhere, if we club all this into the AuthzMgr, we can probably get rid of all AuthzRefresh stuff and it is transparently handled by the AuthzMgr for that role.
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@2297
PS6, Line 2297: addAuthzCache
> Yea I'm finding this logic somewhat hard to follow here as well. Maybe we c
+1
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 23:00:58 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java
File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java:
http://gerrit.cloudera.org:8080/#/c/12748/6/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@133
PS6, Line 133: authzRefresher_.refresh(false);
> Should this also be set in the response object too so that the DDL affects
right, that's more or less what I meant. Most DDL operations return the updated catalog objects to the invoking impalad, and it treats those as if they were delivered topic updates. That's also used for SYNC_DDL.
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 28 Mar 2019 23:37:54 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8293: Support for Ranger cache invalidation
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has abandoned this change. ( http://gerrit.cloudera.org:8080/12748 )
Change subject: IMPALA-8293: Support for Ranger cache invalidation
......................................................................
Abandoned
--
To view, visit http://gerrit.cloudera.org:8080/12748
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I524ee93d09077dd4ff3d18fe517739b7776d01d7
Gerrit-Change-Number: 12748
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Austin Nobis <an...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>