[ 
https://issues.apache.org/jira/browse/HBASE-19920?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16365830#comment-16365830
 ] 

Sean Busbey commented on HBASE-19920:
-------------------------------------

I like the move to the singleton holder idiom. I don't have a specific reason, 
but my intuition is telling me that the two ProtobufUtils should be sharing a 
common place for it though. Is there a reason we can't just hide the holder 
behind something like *DynamicClassloader.getInstance()*?

{code}
/**
- * Dynamic class loader to load filter/comparators
- */
- private final static ClassLoader CLASS_LOADER;
+ * Dynamic class loader to load filter/comparators
+ */
+ private final static class ClassLoaderHolder {
+ private final static ClassLoader CLASS_LOADER;
 
- static {
- ClassLoader parent = ProtobufUtil.class.getClassLoader();
- Configuration conf = HBaseConfiguration.create();
- CLASS_LOADER = new DynamicClassLoader(conf, parent);
+ static {
+ ClassLoader parent = ProtobufUtil.class.getClassLoader();
+ Configuration conf = HBaseConfiguration.create();
+ CLASS_LOADER = new DynamicClassLoader(conf, parent);
+ }
 }
{code}

So long as you're changing this up to do lazy init, should we also clean up how 
we init?

HBC.create can throw (like if the xml config version is off from library). do 
we want that to propagate or should we do a try/catch to set CLASS_LOADER to 
null? AFAICT the use of CLASS_LOADER would then default to the bootstrap 
classloader, which is probably correct.

Also HBC.create expressly sets the classloader for the Configuration to be 
whatever classloader was used to make the HBaseConfiguration instance. Should 
it be created with parent?

{code}

+  private static boolean classLoaderLoaded = false;
+
{code}

This feels wrong. I think this variable needs to be volatile or AtomicBoolean? 
It doesn't have the same guarantees about visibility since it's not within the 
holder, right?

{code}
+    ClassLoader cl = new URLClassLoader(new URL[] { urlPU, urlTU }, 
getClass().getClassLoader());
+
+    Class<?> tokenUtil = cl.loadClass(TokenUtil.class.getCanonicalName());
+    tokenUtil.getDeclaredField("shouldInjectFault").setBoolean(null, true);
{code}

We have to go through all this reflection so that the TokenUtil class 
specifically won't be in the classloader of the test in case some other test 
needed to do something that caused the dynamic classloader to get loaded, right?

Given that, why not also make the fault injection private and/or final and then 
use reflection in the test to allow changing it in the test?

{code}

+    } catch (InvocationTargetException e) {
+      Throwable t = e;
+      boolean serviceExceptionFound = false;
+      while ((t = t.getCause()) != null) {
+        if (t instanceof com.google.protobuf.ServiceException) {
+          serviceExceptionFound = true;
+          break;
+        }
+      }

{code}

Should specifically check that it's the injected ServiceException. I guess by 
text of the message?



> TokenUtil.obtainToken unnecessarily creates a local directory
> -------------------------------------------------------------
>
>                 Key: HBASE-19920
>                 URL: https://issues.apache.org/jira/browse/HBASE-19920
>             Project: HBase
>          Issue Type: Bug
>            Reporter: Rohini Palaniswamy
>            Assignee: Mike Drob
>            Priority: Major
>             Fix For: 2.0
>
>         Attachments: HBASE-19920.patch, HBASE-19920.v2.patch, 
> HBASE-19920.v3.patch, HBASE-19920.v4.patch, HBASE-19920.v5.patch, 
> HBASE-19920.v6.patch, HBASE-19920.v7.patch, HBASE-19920.v8.patch
>
>
> On client code, when one calls TokenUtil.obtainToken it loads ProtobufUtil 
> which in its static block initializes DynamicClassLoader and that creates the 
> directory ${hbase.local.dir}/jars/ and also instantiates a filesystem class 
> to access hbase.dynamic.jars.dir.
> https://github.com/apache/hbase/blob/master/hbase-common/src/main/java/org/apache/hadoop/hbase/util/DynamicClassLoader.java#L109-L127
> Since this is region server specific code, not expecting this to happen when 
> one accesses hbase as a client.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to