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).
##########
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:
:+1: to removing it. Implementers can decide which specific RTE they
want to throw.
##########
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:
That's what I was thinking. I didn't know the specifics.
----------------------------------------------------------------
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:
[email protected]
