You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ranger.apache.org by Madhan Neethiraj <ma...@apache.org> on 2015/09/04 20:02:39 UTC
Re: Review Request 38116: Ranger plugins should not add dependent
libraries to component's CLASSPATH-hdfs-plugin-patch
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/38116/#review97729
-----------------------------------------------------------
Ramesh - I am not done with the review yet. Sending the comments for the review done so fa.
agents-common/scripts/enable-agent.sh (line 39)
<https://reviews.apache.org/r/38116/#comment153740>
"cut -d/ -f5" - this extracts the 5th field. What is the expected input to this method? Does it assume a specific directory level?
ranger-hdfs-plugin-shim/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java (line 37)
<https://reviews.apache.org/r/38116/#comment153741>
If urls is not needed as a class member, it will be good to remove.
ranger-hdfs-plugin-shim/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java (line 69)
<https://reviews.apache.org/r/38116/#comment153742>
I think it will be good to propagate the exception to the caller (in this case namenode). Please review.
ranger-hdfs-plugin-shim/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java (line 88)
<https://reviews.apache.org/r/38116/#comment153743>
Handle rangerHdfsAuthorizerImpl == null.
ranger-hdfs-plugin-shim/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java (line 91)
<https://reviews.apache.org/r/38116/#comment153744>
Consider the following pattern:
if(rangerHdfsAuthorizerImpl != null) {
rangerPluginClassLoader.activate();
try {
rangerHdfsAuthorizerImpl.start();
} finally {
rangerPluginClassLoader.deactivate();
}
}
activate() and deactivate() should set Thread.currentThread().setContextLoader() appropriately.
The same pattern can be used for all calls that get forwarded to rangerHdfsAuthorizerImpl.
ranger-plugin-classloader/src/main/java/org/apache/ranger/plugin/classloader/RangerPluginClassLoaderUtil.java (line 35)
<https://reviews.apache.org/r/38116/#comment153774>
"instance" sounds a better name for this member ("config").
ranger-plugin-classloader/src/main/java/org/apache/ranger/plugin/classloader/RangerPluginClassLoaderUtil.java (line 66)
<https://reviews.apache.org/r/38116/#comment153775>
Should this method be "public"? If not needed, please make this a private method.
- Madhan Neethiraj
On Sept. 4, 2015, 12:53 a.m., Ramesh Mani wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38116/
> -----------------------------------------------------------
>
> (Updated Sept. 4, 2015, 12:53 a.m.)
>
>
> Review request for ranger, Don Bosco Durai, Madhan Neethiraj, Selvamohan Neethiraj, and Velmurugan Periasamy.
>
>
> Repository: ranger
>
>
> Description
> -------
>
> Ranger plugins should not add dependent libraries to component's CLASSPATH-hdfs-plugin-patch
>
>
> Diffs
> -----
>
> agents-common/scripts/enable-agent.sh 16efe74
> agents-common/src/main/java/org/apache/ranger/authorization/hadoop/config/RangerConfiguration.java 0a8907c
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java fa2155c
> hdfs-agent/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizerImpl.java PRE-CREATION
> pom.xml 4b73a61
> ranger-hdfs-plugin-shim/dependency-reduced-pom.xml PRE-CREATION
> ranger-hdfs-plugin-shim/pom.xml PRE-CREATION
> ranger-hdfs-plugin-shim/resources/META-INF/MANIFEST.MF PRE-CREATION
> ranger-hdfs-plugin-shim/src/main/java/org/apache/ranger/authorization/hadoop/RangerHdfsAuthorizer.java PRE-CREATION
> ranger-hdfs-plugin-shim/src/main/resources/META-INF/MANIFEST.MF PRE-CREATION
> ranger-plugin-classloader/pom.xml PRE-CREATION
> ranger-plugin-classloader/src/main/java/org/apache/ranger/plugin/classloader/RangerPluginClassLoader.java PRE-CREATION
> ranger-plugin-classloader/src/main/java/org/apache/ranger/plugin/classloader/RangerPluginClassLoaderUtil.java PRE-CREATION
> ranger-plugin-classloader/src/test/java/org/apache/ranger/plugin/classloader/test/Impl/TestChildFistClassLoader.java PRE-CREATION
> ranger-plugin-classloader/src/test/java/org/apache/ranger/plugin/classloader/test/Impl/TestPluginImpl.java PRE-CREATION
> ranger-plugin-classloader/src/test/java/org/apache/ranger/plugin/classloader/test/Impl/TestPrint.java PRE-CREATION
> ranger-plugin-classloader/src/test/java/org/apache/ranger/plugin/classloader/test/TestPlugin.java PRE-CREATION
> ranger-plugin-classloader/src/test/java/org/apache/ranger/plugin/classloader/test/TestPrint.java PRE-CREATION
> src/main/assembly/hdfs-agent.xml 2c18001
>
> Diff: https://reviews.apache.org/r/38116/diff/
>
>
> Testing
> -------
>
> Manual Testing done
>
>
> Thanks,
>
> Ramesh Mani
>
>