From 8b1a3529580def922b2188a998fdebb65c756de0 Mon Sep 17 00:00:00 2001
From: Jean-Noel Rouvignac <jean-noel.rouvignac@forgerock.com>
Date: Tue, 27 Jan 2015 09:27:30 +0000
Subject: [PATCH] OPENDJ-1716 Various PluggableBackend/Storage refactorings Code review: Nicolas Capponi

---
 opendj3-server-dev/src/server/org/opends/server/backends/persistit/PersistItStorage.java     |   40 ++++++-------
 opendj3-server-dev/src/server/org/opends/server/backends/pluggable/DatabaseContainer.java    |    2 
 opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/ReadableStorage.java  |   42 +++++++++++--
 opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/WriteableStorage.java |   85 ++++++++++++++++++++++++++--
 opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/UpdateFunction.java   |   10 ++-
 5 files changed, 141 insertions(+), 38 deletions(-)

diff --git a/opendj3-server-dev/src/server/org/opends/server/backends/persistit/PersistItStorage.java b/opendj3-server-dev/src/server/org/opends/server/backends/persistit/PersistItStorage.java
index 0dde90e..c4d9dae 100644
--- a/opendj3-server-dev/src/server/org/opends/server/backends/persistit/PersistItStorage.java
+++ b/opendj3-server-dev/src/server/org/opends/server/backends/persistit/PersistItStorage.java
@@ -267,13 +267,13 @@
     }
 
     @Override
-    public void delete(final TreeName treeName, final ByteSequence key)
+    public boolean delete(final TreeName treeName, final ByteSequence key)
     {
       try
       {
         final Exchange ex = getExchangeFromCache(treeName);
         bytesToKey(ex.getKey(), key);
-        ex.remove();
+        return ex.remove();
       }
       catch (final PersistitException e)
       {
@@ -388,21 +388,6 @@
     }
 
     @Override
-    public boolean remove(final TreeName treeName, final ByteSequence key)
-    {
-      try
-      {
-        final Exchange ex = getExchangeFromCache(treeName);
-        bytesToKey(ex.getKey(), key);
-        return ex.remove();
-      }
-      catch (final PersistitException e)
-      {
-        throw new StorageRuntimeException(e);
-      }
-    }
-
-    @Override
     public void renameTree(final TreeName oldTreeName,
         final TreeName newTreeName)
     {
@@ -423,8 +408,7 @@
     }
 
     @Override
-    public void update(final TreeName treeName, final ByteSequence key,
-        final UpdateFunction f)
+    public boolean update(final TreeName treeName, final ByteSequence key, final UpdateFunction f)
     {
       try
       {
@@ -433,8 +417,13 @@
         ex.fetch();
         final ByteSequence oldValue = valueToBytes(ex.getValue());
         final ByteSequence newValue = f.computeNewValue(oldValue);
-        ex.getValue().clear().putByteArray(newValue.toByteArray());
-        ex.store();
+        if (!equals(newValue, oldValue))
+        {
+          ex.getValue().clear().putByteArray(newValue.toByteArray());
+          ex.store();
+          return true;
+        }
+        return false;
       }
       catch (final Exception e)
       {
@@ -442,6 +431,15 @@
       }
     }
 
+    private boolean equals(ByteSequence b1, ByteSequence b2)
+    {
+      if (b1 == null)
+      {
+        return b2 == null;
+      }
+      return b1.equals(b2);
+    }
+
     private Exchange getExchangeFromCache(final TreeName treeName)
         throws PersistitException
     {
diff --git a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/DatabaseContainer.java b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/DatabaseContainer.java
index 8504747..985a58b 100644
--- a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/DatabaseContainer.java
+++ b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/DatabaseContainer.java
@@ -179,7 +179,7 @@
    */
   boolean delete(WriteableStorage txn, ByteSequence key) throws StorageRuntimeException
   {
-    boolean result = txn.remove(treeName, key);
+    final boolean result = txn.delete(treeName, key);
     if (logger.isTraceEnabled())
     {
       logger.trace(messageToLog(result, treeName, txn, key, null));
diff --git a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/ReadableStorage.java b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/ReadableStorage.java
index 5692349..d22e820 100644
--- a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/ReadableStorage.java
+++ b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/ReadableStorage.java
@@ -21,21 +21,49 @@
  * CDDL HEADER END
  *
  *
- *      Copyright 2014 ForgeRock AS
+ *      Copyright 2014-2015 ForgeRock AS
  */
-
 package org.opends.server.backends.pluggable.spi;
 
 import org.forgerock.opendj.ldap.ByteSequence;
 import org.forgerock.opendj.ldap.ByteString;
 
+/**
+ * Represents a readable transaction on a storage engine.
+ */
 public interface ReadableStorage
 {
-  ByteString read(TreeName name, ByteSequence key);
+  /**
+   * Reads the record's value associated to the provided key, in the tree whose name is provided.
+   *
+   * @param treeName
+   *          the tree name
+   * @param key
+   *          the record's key
+   * @return the record's value, or {@code null} if none exists
+   */
+  ByteString read(TreeName treeName, ByteSequence key);
 
-  ByteString getRMW(TreeName name, ByteSequence key);
+  /**
+   * Reads the record's value associated to the provided key in a Read-Modify-Write fashion, in the
+   * tree whose name is provided.
+   *
+   * @param treeName
+   *          the tree name
+   * @param key
+   *          the record's key
+   * @return the record's value, or {@code null} if none exists
+   * @deprecated use {@link #update(TreeName, ByteSequence, UpdateFunction)} instead
+   */
+  @Deprecated
+  ByteString getRMW(TreeName treeName, ByteSequence key);
 
-  Cursor openCursor(TreeName name);
-
-  // TODO: contains, etc.
+  /**
+   * Opens a cursor on the tree whose name is provided.
+   *
+   * @param treeName
+   *          the tree name
+   * @return a new cursor
+   */
+  Cursor openCursor(TreeName treeName);
 }
\ No newline at end of file
diff --git a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/UpdateFunction.java b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/UpdateFunction.java
index 79e8b16..c0de3dd 100644
--- a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/UpdateFunction.java
+++ b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/UpdateFunction.java
@@ -21,12 +21,16 @@
  * CDDL HEADER END
  *
  *
- *      Copyright 2014 ForgeRock AS
+ *      Copyright 2014-2015 ForgeRock AS
  */
 package org.opends.server.backends.pluggable.spi;
 
 import org.forgerock.opendj.ldap.ByteSequence;
 
+/**
+ * Function that computes the new value of a record for a Read-Modify-Write operation inside a transaction.
+ */
+// @FunctionalInterface
 public interface UpdateFunction {
     /**
      * Computes the new value for a record based on the record's existing
@@ -35,8 +39,8 @@
      * @param oldValue
      *            The record's existing content, or {@code null} if the record
      *            does not exist at the moment and is about to be created.
-     * @return The new value for the record, or {@code null} if the record
-     *         should be completely removed.
+     * @return The new value for the record (which may be {@code null} if the record should be removed),
+     *         or the old value if no update is required.
      */
     ByteSequence computeNewValue(ByteSequence oldValue);
 }
diff --git a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/WriteableStorage.java b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/WriteableStorage.java
index 1af739a..96c00ca 100644
--- a/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/WriteableStorage.java
+++ b/opendj3-server-dev/src/server/org/opends/server/backends/pluggable/spi/WriteableStorage.java
@@ -21,29 +21,102 @@
  * CDDL HEADER END
  *
  *
- *      Copyright 2014 ForgeRock AS
+ *      Copyright 2014-2015 ForgeRock AS
  */
 package org.opends.server.backends.pluggable.spi;
 
 import org.forgerock.opendj.ldap.ByteSequence;
 
+/**
+ * Represents a writeable transaction on a storage engine.
+ */
 public interface WriteableStorage extends ReadableStorage
 {
+  /**
+   * Opens the tree having the provided name. The tree is created if does not already exist.
+   *
+   * @param name
+   *          the tree name
+   */
   void openTree(TreeName name);
 
+  /**
+   * Truncates the tree having the provided name. It removes all the records in the tree. 
+   *
+   * @param name
+   *          the tree name
+   */
   void truncateTree(TreeName name);
 
+  /**
+   * Renames the tree from the old to the new name.
+   *
+   * @param oldName
+   *          the old tree name
+   * @param newName
+   *          the new tree name
+   */
   void renameTree(TreeName oldName, TreeName newName);
 
+  /**
+   * Deletes the tree having the provided name.
+   *
+   * @param name
+   *          the tree name
+   */
   void deleteTree(TreeName name);
 
-  void create(TreeName name, ByteSequence key, ByteSequence value);
+  /**
+   * Creates a new record with the provided key and value, in the tree whose name is provided.
+   * If a previous record is associated to the provided key, then it will be replaced by the new record.
+   *
+   * @param treeName
+   *          the tree name
+   * @param key
+   *          the key of the new record
+   * @param value
+   *          the value of the new record
+   */
+  void create(TreeName treeName, ByteSequence key, ByteSequence value);
 
+  /**
+   * Creates a new record with the provided key and value, in the tree whose name is provided, if
+   * the key was not previously associated to any record.
+   * 
+   * @param treeName
+   *          the tree name
+   * @param key
+   *          the key of the new record
+   * @param value
+   *          the value of the new record
+   * @return {@code true} if the new record could be created, {@code false} otherwise
+   * @deprecated use {@link #update(TreeName, ByteSequence, UpdateFunction)} instead
+   */
+  @Deprecated
   boolean putIfAbsent(TreeName treeName, ByteSequence key, ByteSequence value);
 
-  void update(TreeName treeName, ByteSequence key, UpdateFunction f);
+  /**
+   * Updates a record with the provided key according to the new value computed by the update function.
+   *
+   * @param treeName
+   *          the tree name
+   * @param key
+   *          the key of the new record
+   * @param f
+   *          the update function
+   * @return {@code true} if an update was performed, {@code false} otherwise
+   * @see UpdateFunction#computeNewValue(ByteSequence)
+   */
+  boolean update(TreeName treeName, ByteSequence key, UpdateFunction f);
 
-  boolean remove(TreeName name, ByteSequence key);
-
-  void delete(TreeName name, ByteSequence key);
+  /**
+   * Deletes the record with the provided key, in the tree whose name is provided.
+   *
+   * @param treeName
+   *          the tree name
+   * @param key
+   *          the key of the record to delete
+   * @return {@code true} if the record could be deleted, {@code false} otherwise
+   */
+  boolean delete(TreeName treeName, ByteSequence key);
 }
\ No newline at end of file

--
Gitblit v1.10.0