[GitHub] [accumulo] ctubbsii commented on a change in pull request #1905: fixes #1900 Moves volume choosers to SPI

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #1905:
URL: https://github.com/apache/accumulo/pull/1905#discussion_r570289160



##########
File path: core/src/main/java/org/apache/accumulo/core/spi/fs/VolumeChooserEnvironment.java
##########
@@ -0,0 +1,29 @@
+package org.apache.accumulo.core.spi.fs;
+
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
+import org.apache.hadoop.io.Text;
+
+/**
+ * @since 2.1.0
+ */
+public interface VolumeChooserEnvironment {
+  /**
+   * A scope the volume chooser environment; a TABLE scope should be accompanied by a tableId.
+   *
+   * @since 2.1.0
+   */
+  public static enum Scope {
+    DEFAULT, TABLE, INIT, LOGGER
+  }
+
+  public Text getEndRow();
+
+  public boolean hasTableId();
+
+  public TableId getTableId();

Review comment:
       Should these just be combined into:
   
   ```suggestion
     public Optional<TableId> getTableId();
   ```

##########
File path: core/src/main/java/org/apache/accumulo/core/spi/fs/VolumeChooser.java
##########
@@ -0,0 +1,65 @@
+package org.apache.accumulo.core.spi.fs;
+
+import java.util.Set;
+
+import org.apache.accumulo.core.conf.Property;
+
+/**
+ * Helper used to select from a set of Volume URIs. N.B. implementations must be threadsafe.
+ * VolumeChooser.equals will be used for internal caching.
+ *
+ * <p>
+ * Implementations may wish to store configuration in Accumulo's system configuration using the
+ * {@link Property#GENERAL_ARBITRARY_PROP_PREFIX}. They may also benefit from using per-table
+ * configuration using {@link Property#TABLE_ARBITRARY_PROP_PREFIX}.
+ *
+ * @since 2.1.0
+ */
+public interface VolumeChooser {
+
+  /**
+   * Choose a volume from the provided options.
+   *
+   * @param env
+   *          the server environment provided by the calling framework
+   * @param options
+   *          the list of volumes to choose from
+   * @return one of the options
+   * @throws VolumeChooserException
+   *           if there is an error choosing (this is a RuntimeException); this does not preclude
+   *           other RuntimeExceptions from occurring
+   */
+  String choose(VolumeChooserEnvironment env, Set<String> options) throws VolumeChooserException;
+
+  /**
+   * Return the subset of volumes that could possibly be chosen by this chooser across all
+   * invocations of {@link #choose(VolumeChooserEnvironment, Set)}.
+   *
+   * @param env
+   *          the server environment provided by the calling framework
+   * @param options
+   *          the subset of volumes to choose from
+   * @return array of valid options
+   * @throws VolumeChooserException
+   *           if there is an error choosing (this is a RuntimeException); this does not preclude
+   *           other RuntimeExceptions from occurring
+   *
+   */
+  Set<String> choosable(VolumeChooserEnvironment env, Set<String> options)
+      throws VolumeChooserException;
+
+  class VolumeChooserException extends RuntimeException {

Review comment:
       Should this exception extend a more narrow type of RuntimeException to give it some category? Would this be an IllegalStateException, for example?

##########
File path: core/src/main/java/org/apache/accumulo/core/spi/fs/VolumeChooser.java
##########
@@ -0,0 +1,65 @@
+package org.apache.accumulo.core.spi.fs;
+
+import java.util.Set;
+
+import org.apache.accumulo.core.conf.Property;
+
+/**
+ * Helper used to select from a set of Volume URIs. N.B. implementations must be threadsafe.
+ * VolumeChooser.equals will be used for internal caching.
+ *
+ * <p>
+ * Implementations may wish to store configuration in Accumulo's system configuration using the
+ * {@link Property#GENERAL_ARBITRARY_PROP_PREFIX}. They may also benefit from using per-table
+ * configuration using {@link Property#TABLE_ARBITRARY_PROP_PREFIX}.
+ *
+ * @since 2.1.0
+ */
+public interface VolumeChooser {
+
+  /**
+   * Choose a volume from the provided options.
+   *
+   * @param env
+   *          the server environment provided by the calling framework
+   * @param options
+   *          the list of volumes to choose from
+   * @return one of the options
+   * @throws VolumeChooserException
+   *           if there is an error choosing (this is a RuntimeException); this does not preclude
+   *           other RuntimeExceptions from occurring
+   */
+  String choose(VolumeChooserEnvironment env, Set<String> options) throws VolumeChooserException;
+
+  /**
+   * Return the subset of volumes that could possibly be chosen by this chooser across all
+   * invocations of {@link #choose(VolumeChooserEnvironment, Set)}.
+   *
+   * @param env
+   *          the server environment provided by the calling framework
+   * @param options
+   *          the subset of volumes to choose from
+   * @return array of valid options
+   * @throws VolumeChooserException
+   *           if there is an error choosing (this is a RuntimeException); this does not preclude
+   *           other RuntimeExceptions from occurring
+   *
+   */
+  Set<String> choosable(VolumeChooserEnvironment env, Set<String> options)
+      throws VolumeChooserException;

Review comment:
       This javadoc adequately explains what it does, but not what its purpose is. In the SPI, I think it might be important to add a note about why this method exists in its javadoc (a hint about when it might be called).




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org