Integrated feedback from ankgup87

Test Plan: tested using unit tests

Reviewers: ankgup87

Differential Revision: https://reviews.facebook.net/D24573

java/org/rocksdb/MergeOperator.java

@@ -9,11 +9,9 @@ import java.util.*;
 
 /**
  * MergeOperator holds an operator to be applied when compacting
- * two values held under the same key in order to obtain a single
+ * two merge operands held under the same key in order to obtain a single
  * value.
  */
-public abstract class MergeOperator {
+public interface MergeOperator {
-
+    public long newMergeOperatorHandle();
-    abstract protected long newMergeOperatorHandle();
-
 }

java/org/rocksdb/Options.java

@@ -2235,13 +2235,16 @@ public class Options extends RocksObject {
       long handle, int minPartialMergeOperands);
 
   /**
-   * Set the merge operator to be used for merging two different key/value
+   * Set the merge operator to be used for merging two merge operands
-   * pairs that share the same key. The merge function is invoked during
+   * of the same key. The merge function is invoked during
    * compaction and at lookup time, if multiple key/value pairs belonging
    * to the same key are found in the database.
    *
    * @param name the name of the merge function, as defined by
    * the MergeOperators factory (see utilities/MergeOperators.h)
+   * The merge function is specified by name and must be one of the
+   * standard merge operators provided by RocksDB. The available
+   * operators are "put", "uint64add", "stringappend" and "stringappendtest".
    * @return the reference to the current option.
    */
   public Options setMergeOperatorName(String name) {
@@ -2257,8 +2260,7 @@ public class Options extends RocksObject {
    * compaction and at lookup time, if multiple key/value pairs belonging
    * to the same key are found in the database.
    *
-   * @param name the name of the merge function, as defined by
+   * @param a {@link MergeOperator} object
-   * the MergeOperators factory (see utilities/MergeOperators.h)
    * @return the reference to the current option.
    */
   public Options setMergeOperator(MergeOperator mergeOperator) {

java/org/rocksdb/RocksDB.java

@@ -473,7 +473,7 @@ public class RocksDB extends RocksObject {
   }
 
   /**
-   * Set the database entry for "key" to "value".
+   * Add merge operand for key/value pair.
    *
    * @param key the specified key to be merged.
    * @param value the value to be nerged with the current value for
@@ -484,8 +484,9 @@ public class RocksDB extends RocksObject {
   }
 
   /**
-   * Merge the database entry for "key" with "value".
+   * Add merge operand for key/value pair.
    *
+   * @param writeOpts {@link WriteOptions} for this write.
    * @param key the specified key to be merged.
    * @param value the value to be merged with the current value for
    * the specified key.

java/org/rocksdb/StringAppendOperator.java

@@ -6,16 +6,12 @@
 package org.rocksdb;
 
 /**
- * MergeOperator holds an operator to be applied when compacting
+ * StringAppendOperator is a merge operator that concatenates
- * two values held under the same key in order to obtain a single
+ * two strings.
- * value.
  */
-public class StringAppendOperator extends MergeOperator {
+public class StringAppendOperator implements MergeOperator {
-
+    @Override public long newMergeOperatorHandle() {
-	@Override protected long newMergeOperatorHandle() {
+        return newMergeOperatorHandleImpl();
-		return newMergeOperatorHandleImpl();
+    }
-	}
-
     private native long newMergeOperatorHandleImpl();
-
 }

java/org/rocksdb/test/MergeTest.java

@@ -9,80 +9,77 @@ import java.util.Collections;
 import org.rocksdb.*;
 
 public class MergeTest {
-  static final String db_path_string = "/tmp/mergestringjni_db";
+        static final String db_path_string = "/tmp/mergestringjni_db";
-  static final String db_path_function = "/tmp/mergefunctionjni_db";
+        static final String db_path_function = "/tmp/mergefunctionjni_db";
-  static {
+        static {
-	RocksDB.loadLibrary();
+                RocksDB.loadLibrary();
-  }
+        }
 
-  public static void testStringOption()
+        public static void testStringOption()
-	  throws InterruptedException, RocksDBException {
+                throws InterruptedException, RocksDBException {
 
-	System.out.println("Testing merge function string option ===");
+                System.out.println("Testing merge function string option ===");
 
-	Options opt = new Options();
+                Options opt = new Options();
-	opt.setCreateIfMissing(true);
+                opt.setCreateIfMissing(true);
-	opt.setMergeOperatorName("stringappend");
+                opt.setMergeOperatorName("stringappend");
 
-	RocksDB db = RocksDB.open(opt, db_path_string);
+                RocksDB db = RocksDB.open(opt, db_path_string);
 
-	System.out.println("Writing aa under key...");
+                System.out.println("Writing aa under key...");
-	db.put("key".getBytes(), "aa".getBytes());
+                db.put("key".getBytes(), "aa".getBytes());
 
-	System.out.println("Writing bb under key...");
+                System.out.println("Writing bb under key...");
-	db.merge("key".getBytes(), "bb".getBytes());
+                db.merge("key".getBytes(), "bb".getBytes());
 
-	byte[] value = db.get("key".getBytes());
+                byte[] value = db.get("key".getBytes());
-	String strValue = new String(value);
+                String strValue = new String(value);
 
-	System.out.println("Retrieved value: " + strValue);
+                System.out.println("Retrieved value: " + strValue);
 
-	db.close();
+                db.close();
-	opt.dispose();
+                opt.dispose();
 
-	assert(strValue.equals("aa,bb"));
+                assert(strValue.equals("aa,bb"));
 
-	System.out.println("Merge function string option passed!");
+                System.out.println("Merge function string option passed!");
+        }
 
-  }
+        public static void testOperatorOption()
+                throws InterruptedException, RocksDBException {
 
-  public static void testOperatorOption()
+                System.out.println("Testing merge function operator option ===");
-	  throws InterruptedException, RocksDBException {
 
-	System.out.println("Testing merge function operator option ===");
+                Options opt = new Options();
+                opt.setCreateIfMissing(true);
 
-	Options opt = new Options();
+                StringAppendOperator stringAppendOperator = new StringAppendOperator();
-	opt.setCreateIfMissing(true);
+                opt.setMergeOperator(stringAppendOperator);
 
-	StringAppendOperator stringAppendOperator = new StringAppendOperator();
+                RocksDB db = RocksDB.open(opt, db_path_string);
-	opt.setMergeOperator(stringAppendOperator);
 
-	RocksDB db = RocksDB.open(opt, db_path_string);
+                System.out.println("Writing aa under key...");
+                db.put("key".getBytes(), "aa".getBytes());
 
-	System.out.println("Writing aa under key...");
+                System.out.println("Writing bb under key...");
-	db.put("key".getBytes(), "aa".getBytes());
+                db.merge("key".getBytes(), "bb".getBytes());
 
-	System.out.println("Writing bb under key...");
+                byte[] value = db.get("key".getBytes());
-	db.merge("key".getBytes(), "bb".getBytes());
+                String strValue = new String(value);
 
-	byte[] value = db.get("key".getBytes());
+                System.out.println("Retrieved value: " + strValue);
-	String strValue = new String(value);
 
-	System.out.println("Retrieved value: " + strValue);
+                db.close();
+                opt.dispose();
 
-	db.close();
+                assert(strValue.equals("aa,bb"));
-	opt.dispose();
 
-	assert(strValue.equals("aa,bb"));
+                System.out.println("Merge function operator option passed!");
+        }
 
-	System.out.println("Merge function operator option passed!");
+        public static void main(String[] args)
-
+                throws InterruptedException, RocksDBException {
-  }
+                testStringOption();
-
+                testOperatorOption();
-  public static void main(String[] args)
+        }
-	  throws InterruptedException, RocksDBException {
-	testStringOption();
-	testOperatorOption();
-
-  }
 }

java/rocksjni/merge_operator.cc

@@ -3,7 +3,8 @@
 // LICENSE file in the root directory of this source tree. An additional grant
 // of patent rights can be found in the PATENTS file in the same directory.
 //
-// This file implements the "bridge" between Java and C++ for rocksdb::MergeOperator.
+// This file implements the "bridge" between Java and C++
+// for rocksdb::MergeOperator.
 
 #include <stdio.h>
 #include <stdlib.h>
@@ -27,9 +28,10 @@
  * Method:    newMergeOperatorHandle
  * Signature: ()J
  */
-jlong Java_org_rocksdb_StringAppendOperator_newMergeOperatorHandleImpl(JNIEnv* env, jobject jobj) {
+jlong Java_org_rocksdb_StringAppendOperator_newMergeOperatorHandleImpl
-  std::shared_ptr<rocksdb::MergeOperator> *op = new std::shared_ptr<rocksdb::MergeOperator>();
+(JNIEnv* env, jobject jobj) {
+  std::shared_ptr<rocksdb::MergeOperator> *op =
+    new std::shared_ptr<rocksdb::MergeOperator>();
   *op = rocksdb::MergeOperators::CreateFromStringId("stringappend");
   return reinterpret_cast<jlong>(op);
 }
-

java/rocksjni/options.cc

@@ -1623,9 +1623,10 @@ void Java_org_rocksdb_Options_setMergeOperatorName(
  * Signature: (JJjava/lang/String)V
  */
 void Java_org_rocksdb_Options_setMergeOperator(
-    JNIEnv* env, jobject jobj, jlong jhandle, jlong mergeOperatorHandle) {
+  JNIEnv* env, jobject jobj, jlong jhandle, jlong mergeOperatorHandle) {
   reinterpret_cast<rocksdb::Options*>(jhandle)->merge_operator =
-    *(reinterpret_cast<std::shared_ptr<rocksdb::MergeOperator>*> (mergeOperatorHandle));
+    *(reinterpret_cast<std::shared_ptr<rocksdb::MergeOperator>*>
+      (mergeOperatorHandle));
 }
 
 //////////////////////////////////////////////////////////////////////////////
@@ -1784,4 +1785,3 @@ void Java_org_rocksdb_ReadOptions_setTailing(
   reinterpret_cast<rocksdb::ReadOptions*>(jhandle)->tailing =
       static_cast<bool>(jtailing);
 }
-