Address PR comments.

Author: rhubner

java/rocksjni/event_listener.cc

@@ -34,7 +34,7 @@ jlong Java_org_rocksdb_AbstractEventListener_createNewEventListener(
                 env, jobj, enabled_event_callbacks));
     return GET_CPLUSPLUS_POINTER(sptr_event_listener);
   } catch (ROCKSDB_NAMESPACE::JniException&) {
-    // We always throw JniException with Java Exception
+    // JNIException indicates that a Java Exception has been thrown in the env
     return 0;
   }
 }

java/rocksjni/portal.h

@@ -489,9 +489,14 @@ class StatusJni
     return construct(env, status, jclazz, mid);
   }
 
-  // StatusJni can't cache jclass because in descructor we need JniUtil,
-  // JniUtil need RocksDBExceptionJni, and RocksDBExceptionJni need StatusJni.
-  // So I can't even change the order of the classes in portal.h
+  /**
+   * This method is used in situations when we need to create
+   * org.rocksdb.Status in C++ rocksDB callback. In situation when
+   * Java code was loaded by a custom class loader,
+   * env->FindClass will fail, because on the JVM attached thread we have only
+   * system class loader. For more info :
+   * https://docs.oracle.com/javase/8/docs/technotes/guides/jni/spec/functions.html#FindClass
+   */
   static jobject construct(JNIEnv* env, const Status& status, jclass jclazz,
                            jmethodID mid) {
     // convert the Status state for Java
@@ -2513,7 +2518,7 @@ class JniUtil {
     auto status = env->GetJavaVM(vm);
     if (status != 0) {
       ROCKSDB_NAMESPACE::JniException::ThrowNew(
-          env, "Unable to retrieve instance ov JavaVM");
+          env, "Unable to retrieve instance of JavaVM");
     }
   }
 };

java/src/test/java/org/rocksdb/EventListenerClassloaderTest.java

@@ -18,29 +18,36 @@
 public class EventListenerClassloaderTest {
   @Test
   public void testCallback() throws Exception {
-    try {
-      this.getClass().getClassLoader().loadClass("org.rocksdb.RocksDB");
-      fail("It looks like RocksDB is on classpath. This test must load RocksDB via custom "
-          + "classLoader"
-          + " to verify that callback cache all class instances.");
-    } catch (ClassNotFoundException e) {
-      ;
-    }
+    assertThat(hasRocksDBonPath())
+        .as("It looks like RocksDB is on classpath. "
+            + "This test must load RocksDB via custom "
+            + "classLoader to verify that callback cache all class instances.")
+        .isFalse();
 
     String jarPath = System.getProperty("rocks-jar");
-    assertThat(jarPath).isNotNull().as("Java property 'rocks-jar' was not setup properly");
+    assertThat(jarPath).as("Java property 'rocks-jar' was not setup properly").isNotNull();
 
     Path classesDir = Paths.get(jarPath);
-    ClassLoader cl = new URLClassLoader(new URL[] {classesDir.toAbsolutePath().toUri().toURL()});
+    try (URLClassLoader cl =
+             new URLClassLoader(new URL[] {classesDir.toAbsolutePath().toUri().toURL()})) {
+      Class<?> rocksDBclazz = cl.loadClass("org.rocksdb.RocksDB");
+      Method loadLibrary = rocksDBclazz.getMethod("loadLibrary");
+      loadLibrary.invoke(null);
 
-    Class rocksDBclazz = cl.loadClass("org.rocksdb.RocksDB");
-    Method loadLibrary = rocksDBclazz.getMethod("loadLibrary");
-    loadLibrary.invoke(null);
+      Class<?> testableEventListenerClazz = cl.loadClass("org.rocksdb.test.TestableEventListener");
+      Method invokeAllCallbacksInThread =
+          testableEventListenerClazz.getMethod("invokeAllCallbacksInThread");
+      Object instance = testableEventListenerClazz.getDeclaredConstructor().newInstance();
+      invokeAllCallbacksInThread.invoke(instance);
+    }
+  }
 
-    Class testableEventListenerClazz = cl.loadClass("org.rocksdb.test.TestableEventListener");
-    Method invokeAllCallbacksInThread =
-        testableEventListenerClazz.getMethod("invokeAllCallbacksInThread");
-    Object instance = testableEventListenerClazz.getDeclaredConstructor().newInstance();
-    invokeAllCallbacksInThread.invoke(instance);
+  private boolean hasRocksDBonPath() {
+    try {
+      this.getClass().getClassLoader().loadClass("org.rocksdb.RocksDB");
+      return true;
+    } catch (ClassNotFoundException e) {
+      return false;
+    }
   }
 }