jerryshao commented on code in PR #4744: URL: https://github.com/apache/gravitino/pull/4744#discussion_r1759610420
########## authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationConfig.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.authorization.ranger; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.gravitino.authorization.Privilege; + +/** + * Ranger authorization use this configuration to mapping Gravitino privilege to the Ranger Review Comment: I need to think a bit on this, my initial feeling is that This `RangerAuthorizationConfig` may not be easy for people to use. Most of the interfaces like `initializePrivilegesMappingConfig()` is hard to people to understand from the definition. People doesn't know what should be implemented, what is the input, output and side-effect of this interface. Without knowing this, it is hard for the developers to extend or implement this interface. ########## authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivilege.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.authorization.ranger; + +/** RangerPrivilege interface is used to define the Ranger privileges. */ +public interface RangerPrivilege { + String toString(); + + boolean equals(String value); + + /** Ranger Hive privileges enumeration. */ + enum RangerHivePrivilege implements RangerPrivilege { + ALL("all"), + SELECT("select"), + UPDATE("update"), + CREATE("create"), + DROP("drop"), + ALTER("alter"), + INDEX("index"), + LOCK("lock"), + READ("read"), + WRITE("write"), + REPLADMIN("repladmin"), + SERVICEADMIN("serviceadmin"); + + private final String string; // Access a type in the Ranger policy item Review Comment: It is better to have a different name for this class variable, not similar to the reserved word. ########## authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationConfig.java: ########## @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.authorization.ranger; + +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import org.apache.gravitino.authorization.Privilege; + +/** + * Ranger authorization use this configuration to mapping Gravitino privilege to the Ranger Review Comment: It's OK for internal use. But if we want to expose to the developers to extend, seems the definition is not self-explanatory. The concept of `xxxConfig` makes people hard to understand without context. ########## authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerPrivilege.java: ########## @@ -0,0 +1,81 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.gravitino.authorization.ranger; + +/** RangerPrivilege interface is used to define the Ranger privileges. */ +public interface RangerPrivilege { + String toString(); + + boolean equals(String value); Review Comment: the interface name `equals` is conflicted with Java's built-in `equals` method name, also for `toString`, it is better to have another name. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
