yuqi1129 commented on code in PR #6211:
URL: https://github.com/apache/gravitino/pull/6211#discussion_r2015944578
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java:
##########
@@ -278,8 +278,9 @@ private void
removeSchemaMetadataObject(AuthorizationMetadataObject authzMetadat
authzMetadataObject instanceof PathBasedMetadataObject,
"The metadata object must be a PathBasedMetadataObject");
Preconditions.checkArgument(
- authzMetadataObject.type()
- ==
PathBasedMetadataObject.PathType.get(MetadataObject.Type.SCHEMA),
+ authzMetadataObject
+ .type()
+
.equals(PathBasedMetadataObject.PathType.get(MetadataObject.Type.SCHEMA)),
Review Comment:
Why don't you create some static instances in PathBasedMetadataObject to
avoid creating new instances every time?
```java
public class PathBasedMetadataObject {
public static final SCHEMA_PATH = new PathType(MetadataObject.Type.SCHEMA);
public static final TABLE_PATH = new PathType(MetadataObject.Type.TABLE);
public static final FILESET_PATH = new PathType(MetadataObject.Type.FILESET);
...
}
```
Then we can use `SCHEMA_PATH`, `TABLE_PATH` and `FILESET_PATH` directly.
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationHDFSPlugin.java:
##########
@@ -499,27 +506,32 @@ public List<AuthorizationSecurableObject>
translatePrivilege(SecurableObject sec
case CREATE_SCHEMA:
switch (securableObject.type()) {
case METALAKE:
+ NameIdentifier[] catalogs =
+ GravitinoEnv.getInstance()
+ .catalogDispatcher()
+ .listCatalogs(Namespace.of(identifier.name()));
+ for (NameIdentifier catalog : catalogs) {
+ AuthorizationUtils.getMetadataObjectLocation(
+ catalog, Entity.EntityType.CATALOG)
+ .forEach(
+ locationPath ->
+ createPathBasedMetadataObject(
+ securableObject,
+ locationPath,
+ rangerSecurableObjects,
+ rangerPrivileges));
+ }
+ break;
Review Comment:
This part is almost the same as L470. Can you try to extract a common method?
##########
core/src/main/java/org/apache/gravitino/authorization/AuthorizationUtils.java:
##########
@@ -465,30 +471,72 @@ public static List<String> getMetadataObjectLocation(
}
break;
case SCHEMA:
- {
- Catalog catalogObj =
- GravitinoEnv.getInstance()
- .catalogDispatcher()
- .loadCatalog(
- NameIdentifier.of(ident.namespace().level(0),
ident.namespace().level(1)));
- LOG.info("Catalog provider is %s", catalogObj.provider());
- if (catalogObj.provider().equals("hive")) {
- Schema schema =
GravitinoEnv.getInstance().schemaDispatcher().loadSchema(ident);
- if (schema.properties().containsKey(HiveConstants.LOCATION)) {
+ Catalog catalogObj =
+ GravitinoEnv.getInstance()
+ .catalogDispatcher()
+ .loadCatalog(
+ NameIdentifier.of(ident.namespace().level(0),
ident.namespace().level(1)));
+ Schema schema =
GravitinoEnv.getInstance().schemaDispatcher().loadSchema(ident);
+
+ switch (catalogObj.type()) {
+ case RELATIONAL:
+ LOG.info("Catalog provider is {}", catalogObj.provider());
Review Comment:
+1
##########
core/src/test/java/org/apache/gravitino/utils/TestNamespaceUtil.java:
##########
@@ -18,12 +18,32 @@
*/
package org.apache.gravitino.utils;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
import org.apache.gravitino.Namespace;
import org.apache.gravitino.exceptions.IllegalNamespaceException;
+import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
public class TestNamespaceUtil {
+ private final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
Review Comment:
Why do we need to use the stream?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@gravitino.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
