[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

classic Classic list List threaded Threaded
74 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
GitHub user ctubbsii opened a pull request:

    https://github.com/apache/accumulo/pull/253

    ACCUMULO-4086 Improve volume chooser fallback

    Implement a better fallback mechanism when the user's selected
    VolumeChooser implementation cannot be loaded or is not specified.
    Handles all such cases, including non-table scopes (logger), and
    per-table scopes.
   
    ****
   
    Note: this isn't quite ready for merging, but since I'm trying to proxy code written by somebody else, I think it'd be helpful to have it inspected/reviewed by more eyes. I tried to run the tests, but they hung (in Eclipse, at least). Keep in mind that this has been rebase'd from an older version of Accumulo, and probably needs a bit more work.
   
    The goal of this is described in [ACCUMULO-4086][1] and its parent task, but all I have is the implementation, and it's not entirely clear what the best design is, in order to make sure this particular implementation matches that design.
   
    Any and all feedback/assistance is helpful. However, I'm already aware documentation is significantly lacking.... will address that once the design and implementation is hammered out... and that's a bit fuzzy to me right now. For example, I see that the patch modifies the default configs, and it's not clear to me what design goal that achieves.
   
    [1]: https://issues.apache.org/jira/browse/ACCUMULO-4086

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/ctubbsii/accumulo ACCUMULO-4086-volume-chooser-fallback

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/accumulo/pull/253.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #253
   
----
commit 1c81d6e44a4127ae74852bc8246396878d05cef6
Author: Christopher Tubbs <[hidden email]>
Date:   2017-04-21T19:41:57Z

    ACCUMULO-4086 Improve volume chooser fallback
   
    Implement a better fallback mechanism when the user's selected
    VolumeChooser implementation cannot be loaded or is not specified.
    Handles all such cases, including non-table scopes (logger), and
    per-table scopes.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo issue #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/253
 
    I spoke with Matt, who wrote the original code for this PR, to get a sense of his design intentions. He clarified that the overall goal was to try to avoid `RandomVolumeChooser` as much as possible, no matter what went wrong (typos, etc.) when a user mis-typed a per-table chooser class or the class path was not set up correctly to find the user's chooser class.
   
    Currently (without this PR), Accumulo has hard-coded defaults on `RandomVolumeChooser` in several places. This can be problematic, because it can result in filling up volumes reserved for certain tables or the WALs. Further, it seems to select the `RandomVolumeChooser` mostly silently.
   
    This PR changes things so that if the user has configured the `PerTableVolumeChooser`, then when that fails to load a particular table's chooser (`table.volume.chooser` from a `TableConfiguration`), it will not fall back to the `RandomVolumeChooser`, but instead fall back to the system-wide setting (`SystemConfiguration`) for `table.volume.chooser` (for example, appearing in `accumulo-site.xml`, outside of any per-table context). If the user did not specify a `table.volume.chooser`, then then the `PerTableVolumeChooser` would simply fail. This ensures that `RandomVolumeChooser` is never used accidentally when using the `PerTableVolumeChooser`. A consequence of the change in this PR is that the defaults have changed, but this will only affect users who were relying on `general.volume.chooser` being the default and overriding `table.volume.chooser` (or vice-versa).
   
    The following is how I'm thinking it should probably work:
    Use the `RandomVolumeChooser` as the new default for `general.volume.chooser` and empty string (or null) for the default `table.volume.chooser`, as they are in this PR (see last sentence of previous paragraph). Document *VERY WELL* (especially in the release notes) that if the `PerTableVolumeChooser` is required by the user (e.g. they were relying on it being the default), then the user **must** set the `general.volume.chooser` to the `PerTableVolumeChooser` **AND** must either set a default value for `table.volume.chooser` in the system-wide configuration (system-wide ZK settings, or site file) **OR** ensure `table.volume.chooser` is set for *every* table (or namespace). If loading a per-table chooser or the general chooser fails at any point, we simply throw an exception, rather than trying to find a fall-back chooser to use.
   
    If we simply fail, rather than trying to fall back to some system-wide working configuration, we can still run into problems loading the class defined at that level, and it's not clear that is what the user wishes to occur. So, we should just fail instead. This would simplify this PR a bit.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo issue #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on the issue:

    https://github.com/apache/accumulo/pull/253
 
    I concur that failing if a configured class fails to load is better then falling back to something else.  If a user went through the trouble of configuring something and they did it incorrectly, then the system should never fall back to something else because that may have unexpected and perhaps bad consequences.  Instead the user should be forced to fix the configuration.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113243125
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java ---
    @@ -18,36 +18,65 @@
     
     import java.util.concurrent.ConcurrentHashMap;
     
    +import org.apache.accumulo.core.client.AccumuloException;
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
     import org.apache.accumulo.server.conf.TableConfiguration;
    +import org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
     
     /**
      * A {@link VolumeChooser} that delegates to another volume chooser based on the presence of an experimental table property,
      * {@link Property#TABLE_VOLUME_CHOOSER}. If it isn't found, defaults back to {@link RandomVolumeChooser}.
      */
     public class PerTableVolumeChooser implements VolumeChooser {
    -
    -  private final VolumeChooser fallbackVolumeChooser = new RandomVolumeChooser();
    +  private static final Logger log = LoggerFactory.getLogger(PerTableVolumeChooser.class);
       // TODO Add hint of expected size to construction, see ACCUMULO-3410
       /* Track VolumeChooser instances so they can keep state. */
       private final ConcurrentHashMap<String,VolumeChooser> tableSpecificChooser = new ConcurrentHashMap<>();
    +  private final ConcurrentHashMap<String,VolumeChooser> scopeSpecificChooser = new ConcurrentHashMap<>();
    +  private final RandomVolumeChooser randomChooser = new RandomVolumeChooser();
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
    +  private volatile VolumeChooser fallbackVolumeChooser = null;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    VolumeChooser chooser = null;
    -    if (env.hasTableId()) {
    -      // This local variable is an intentional component of the single-check idiom.
    -      ServerConfigurationFactory localConf = serverConfs;
    -      if (localConf == null) {
    -        // If we're under contention when first getting here we'll throw away some initializations.
    -        localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -        serverConfs = localConf;
    -      }
    -      final TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (log.isTraceEnabled()) {
    +      log.trace("PerTableVolumeChooser.choose");
    +    }
    +    VolumeChooser chooser;
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // Should only get here during Initialize. Configurations are not yet available.
    +      return randomChooser.choose(env, options);
    +    }
    +
    +    ServerConfigurationFactory localConf = loadConf();
    +    lazilyCreateFallbackChooser();
    +    if (env.hasScope()) {
    --- End diff --
   
    Perhaps ignorance on my part, but shouldn't env.hasTableId() be checked first?  Can env.hasTableId() && env.hasScope()?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113241495
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java ---
    @@ -21,84 +21,90 @@
     import java.util.ArrayList;
     import java.util.Arrays;
     import java.util.Collections;
    -import java.util.HashMap;
     import java.util.HashSet;
     import java.util.Map;
     import java.util.Set;
    -import java.util.function.Predicate;
     
    +import org.apache.accumulo.core.client.AccumuloException;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.core.volume.Volume;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
    -import org.apache.accumulo.server.conf.TableConfiguration;
     import org.apache.commons.collections.map.LRUMap;
     import org.apache.commons.lang.StringUtils;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     /**
      * A {@link RandomVolumeChooser} that limits its choices from a given set of options to the subset of those options preferred for a particular table. Defaults
    - * to selecting from all of the options presented. Can be customized via the table property {@value #PREFERRED_VOLUMES_CUSTOM_KEY}, which should contain a comma
    + * to selecting from all of the options presented. Can be customized via the table property table.custom.preferredVolumes, which should contain a comma
      * separated list of {@link Volume} URIs. Note that both the property name and the format of its value are specific to this particular implementation.
      */
     public class PreferredVolumeChooser extends RandomVolumeChooser {
       private static final Logger log = LoggerFactory.getLogger(PreferredVolumeChooser.class);
     
    -  /**
    -   * This should match {@link Property#TABLE_ARBITRARY_PROP_PREFIX}
    -   */
    -  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = "table.custom.preferredVolumes";
    -  // TODO ACCUMULO-3417 replace this with the ability to retrieve by String key.
    -  private static final Predicate<String> PREFERRED_VOLUMES_FILTER = key -> PREFERRED_VOLUMES_CUSTOM_KEY.equals(key);
    +  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = Property.TABLE_ARBITRARY_PROP_PREFIX + "preferredVolumes";
     
       @SuppressWarnings("unchecked")
       private final Map<String,Set<String>> parsedPreferredVolumes = Collections.synchronizedMap(new LRUMap(1000));
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    if (!env.hasTableId())
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // this should only happen during initialize
    +      log.warn("No table id or scope, so it's not possible to determine preferred volumes.  Using all volumes.");
           return super.choose(env, options);
    +    }
    +    ServerConfigurationFactory localConf = loadConf();
     
    -    // Get the current table's properties, and find the preferred volumes property
    -    // This local variable is an intentional component of the single-check idiom.
    -    ServerConfigurationFactory localConf = serverConfs;
    -    if (localConf == null) {
    -      // If we're under contention when first getting here we'll throw away some initializations.
    -      localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -      serverConfs = localConf;
    +    String systemPreferredVolumes = localConf.getConfiguration().get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    if (null == systemPreferredVolumes || systemPreferredVolumes.isEmpty()) {
    +      String logMessage = "Default preferred volumes are missing.";
    +      log.debug(logMessage);
    +      throw new AccumuloException(logMessage);
         }
    -    TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    -    final Map<String,String> props = new HashMap<>();
    -    tableConf.getProperties(props, PREFERRED_VOLUMES_FILTER);
    -    if (props.isEmpty()) {
    -      log.warn("No preferred volumes specified. Defaulting to randomly choosing from instance volumes");
    -      return super.choose(env, options);
    +
    +    String volumes = null;
    +    if (env.hasTableId()) {
    +      volumes = localConf.getTableConfiguration(env.getTableId()).get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    } else if (env.hasScope()) {
    +      volumes = localConf.getConfiguration().get(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".preferredVolumes");
    +    }
    +
    +    // if there was an empty or missing property, use the system-wide volumes
    +    if (null == volumes || volumes.isEmpty()) {
    +      if (env.hasTableId()) {
    +        log.warn("Missing property for TableID " + env.getTableId() + " but it should have picked up default volumes.");
    +      } else {
    +        log.debug("Missing preferred volumes for scope " + env.getScope() + ". Using default volumes.");
    --- End diff --
   
    Why is this debug?  Should the expected property name be in the message?  Should this fail instead of log?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113243239
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -191,7 +191,7 @@
           + "server-internal scheduled tasks"),
       // If you update the default type, be sure to update the default used for initialization failures in VolumeManagerImpl
       @Experimental
    -  GENERAL_VOLUME_CHOOSER("general.volume.chooser", "org.apache.accumulo.server.fs.PerTableVolumeChooser", PropertyType.CLASSNAME,
    +  GENERAL_VOLUME_CHOOSER("general.volume.chooser", "org.apache.accumulo.server.fs.RandomVolumeChooser", PropertyType.CLASSNAME,
    --- End diff --
   
    Why do the defaults need to change?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113241222
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java ---
    @@ -21,84 +21,90 @@
     import java.util.ArrayList;
     import java.util.Arrays;
     import java.util.Collections;
    -import java.util.HashMap;
     import java.util.HashSet;
     import java.util.Map;
     import java.util.Set;
    -import java.util.function.Predicate;
     
    +import org.apache.accumulo.core.client.AccumuloException;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.core.volume.Volume;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
    -import org.apache.accumulo.server.conf.TableConfiguration;
     import org.apache.commons.collections.map.LRUMap;
     import org.apache.commons.lang.StringUtils;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     /**
      * A {@link RandomVolumeChooser} that limits its choices from a given set of options to the subset of those options preferred for a particular table. Defaults
    - * to selecting from all of the options presented. Can be customized via the table property {@value #PREFERRED_VOLUMES_CUSTOM_KEY}, which should contain a comma
    + * to selecting from all of the options presented. Can be customized via the table property table.custom.preferredVolumes, which should contain a comma
      * separated list of {@link Volume} URIs. Note that both the property name and the format of its value are specific to this particular implementation.
      */
     public class PreferredVolumeChooser extends RandomVolumeChooser {
       private static final Logger log = LoggerFactory.getLogger(PreferredVolumeChooser.class);
     
    -  /**
    -   * This should match {@link Property#TABLE_ARBITRARY_PROP_PREFIX}
    -   */
    -  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = "table.custom.preferredVolumes";
    -  // TODO ACCUMULO-3417 replace this with the ability to retrieve by String key.
    -  private static final Predicate<String> PREFERRED_VOLUMES_FILTER = key -> PREFERRED_VOLUMES_CUSTOM_KEY.equals(key);
    +  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = Property.TABLE_ARBITRARY_PROP_PREFIX + "preferredVolumes";
     
       @SuppressWarnings("unchecked")
       private final Map<String,Set<String>> parsedPreferredVolumes = Collections.synchronizedMap(new LRUMap(1000));
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    if (!env.hasTableId())
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // this should only happen during initialize
    +      log.warn("No table id or scope, so it's not possible to determine preferred volumes.  Using all volumes.");
           return super.choose(env, options);
    +    }
    +    ServerConfigurationFactory localConf = loadConf();
     
    -    // Get the current table's properties, and find the preferred volumes property
    -    // This local variable is an intentional component of the single-check idiom.
    -    ServerConfigurationFactory localConf = serverConfs;
    -    if (localConf == null) {
    -      // If we're under contention when first getting here we'll throw away some initializations.
    -      localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -      serverConfs = localConf;
    +    String systemPreferredVolumes = localConf.getConfiguration().get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    if (null == systemPreferredVolumes || systemPreferredVolumes.isEmpty()) {
    +      String logMessage = "Default preferred volumes are missing.";
    +      log.debug(logMessage);
    +      throw new AccumuloException(logMessage);
         }
    -    TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    -    final Map<String,String> props = new HashMap<>();
    -    tableConf.getProperties(props, PREFERRED_VOLUMES_FILTER);
    -    if (props.isEmpty()) {
    -      log.warn("No preferred volumes specified. Defaulting to randomly choosing from instance volumes");
    -      return super.choose(env, options);
    +
    +    String volumes = null;
    +    if (env.hasTableId()) {
    +      volumes = localConf.getTableConfiguration(env.getTableId()).get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    } else if (env.hasScope()) {
    +      volumes = localConf.getConfiguration().get(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".preferredVolumes");
    +    }
    +
    +    // if there was an empty or missing property, use the system-wide volumes
    +    if (null == volumes || volumes.isEmpty()) {
    +      if (env.hasTableId()) {
    +        log.warn("Missing property for TableID " + env.getTableId() + " but it should have picked up default volumes.");
    --- End diff --
   
    Could include `table.custom.preferredVolumes` in the log message.
   
    Why log a warn instead of throw an exception here?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user keith-turner commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113242911
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java ---
    @@ -21,84 +21,90 @@
     import java.util.ArrayList;
     import java.util.Arrays;
     import java.util.Collections;
    -import java.util.HashMap;
     import java.util.HashSet;
     import java.util.Map;
     import java.util.Set;
    -import java.util.function.Predicate;
     
    +import org.apache.accumulo.core.client.AccumuloException;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.core.volume.Volume;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
    -import org.apache.accumulo.server.conf.TableConfiguration;
     import org.apache.commons.collections.map.LRUMap;
     import org.apache.commons.lang.StringUtils;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     /**
      * A {@link RandomVolumeChooser} that limits its choices from a given set of options to the subset of those options preferred for a particular table. Defaults
    - * to selecting from all of the options presented. Can be customized via the table property {@value #PREFERRED_VOLUMES_CUSTOM_KEY}, which should contain a comma
    + * to selecting from all of the options presented. Can be customized via the table property table.custom.preferredVolumes, which should contain a comma
      * separated list of {@link Volume} URIs. Note that both the property name and the format of its value are specific to this particular implementation.
      */
     public class PreferredVolumeChooser extends RandomVolumeChooser {
       private static final Logger log = LoggerFactory.getLogger(PreferredVolumeChooser.class);
     
    -  /**
    -   * This should match {@link Property#TABLE_ARBITRARY_PROP_PREFIX}
    -   */
    -  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = "table.custom.preferredVolumes";
    -  // TODO ACCUMULO-3417 replace this with the ability to retrieve by String key.
    -  private static final Predicate<String> PREFERRED_VOLUMES_FILTER = key -> PREFERRED_VOLUMES_CUSTOM_KEY.equals(key);
    +  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = Property.TABLE_ARBITRARY_PROP_PREFIX + "preferredVolumes";
     
       @SuppressWarnings("unchecked")
       private final Map<String,Set<String>> parsedPreferredVolumes = Collections.synchronizedMap(new LRUMap(1000));
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    if (!env.hasTableId())
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // this should only happen during initialize
    +      log.warn("No table id or scope, so it's not possible to determine preferred volumes.  Using all volumes.");
           return super.choose(env, options);
    +    }
    +    ServerConfigurationFactory localConf = loadConf();
     
    -    // Get the current table's properties, and find the preferred volumes property
    -    // This local variable is an intentional component of the single-check idiom.
    -    ServerConfigurationFactory localConf = serverConfs;
    -    if (localConf == null) {
    -      // If we're under contention when first getting here we'll throw away some initializations.
    -      localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -      serverConfs = localConf;
    +    String systemPreferredVolumes = localConf.getConfiguration().get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    if (null == systemPreferredVolumes || systemPreferredVolumes.isEmpty()) {
    +      String logMessage = "Default preferred volumes are missing.";
    +      log.debug(logMessage);
    +      throw new AccumuloException(logMessage);
         }
    -    TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    -    final Map<String,String> props = new HashMap<>();
    -    tableConf.getProperties(props, PREFERRED_VOLUMES_FILTER);
    -    if (props.isEmpty()) {
    -      log.warn("No preferred volumes specified. Defaulting to randomly choosing from instance volumes");
    -      return super.choose(env, options);
    +
    +    String volumes = null;
    +    if (env.hasTableId()) {
    +      volumes = localConf.getTableConfiguration(env.getTableId()).get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    } else if (env.hasScope()) {
    +      volumes = localConf.getConfiguration().get(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".preferredVolumes");
    --- End diff --
   
    Would it make sense to add static that takes a scope argument and generated this property?  This could be a public static equivalent to `PREFERRED_VOLUMES_CUSTOM_KEY` for configuration purposes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113243918
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java ---
    @@ -68,10 +100,107 @@ public String choose(VolumeChooserEnvironment env, String[] options) {
               chooser = last;
             }
           }
    +    }
    +    return chooser;
    +  }
    +
    +  private VolumeChooser getVolumeChooserForNonTable(VolumeChooserEnvironment env, ServerConfigurationFactory localConf) {
    +    VolumeChooser chooser;
    +    final String customProperty = Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".volume.chooser";
    +
    +    if (log.isTraceEnabled()) {
    +      log.trace("Scope: " + env.getScope());
    +      log.trace("Looking up property: " + customProperty);
    +    }
    +
    +    AccumuloConfiguration systemConfiguration = localConf.getConfiguration();
    +    String clazz = systemConfiguration.get(customProperty);
    +    if (null == clazz || clazz.isEmpty()) {
    +      log.debug("Property not found: " + customProperty + " using fallback chooser.");
    +      return fallbackVolumeChooser;
         } else {
    -      chooser = fallbackVolumeChooser;
    +      chooser = scopeSpecificChooser.get(env.getScope());
    +      if (chooser == null) {
    +        VolumeChooser temp;
    +        try {
    +          temp = loadClassForCustomProperty(clazz);
    +        } catch (Exception e) {
    +          log.error("Failed to create instance for " + env.getScope() + " configured to use " + clazz + " via " + customProperty);
    +          return fallbackVolumeChooser;
    +        }
    +        chooser = scopeSpecificChooser.putIfAbsent(env.getScope(), temp);
    +        if (chooser == null) {
    +          chooser = temp;
    +          // Otherwise, someone else beat us to initializing; use theirs.
    +        }
    +      } else if (!(chooser.getClass().getName().equals(clazz))) {
    +        if (log.isTraceEnabled()) {
    +          log.trace("change detected for scope: " + env.getScope());
    +        }
    +        // the configuration for this scope's chooser has been updated. In the case of failure to instantiate we'll repeat here next call.
    +        // TODO stricter definition of when the updated property is used, ref ACCUMULO-3412
    +        VolumeChooser temp;
    +        try {
    +          temp = loadClassForCustomProperty(clazz);
    +        } catch (Exception e) {
    +          log.error("Failed to create instance for " + env.getScope() + " configured to use " + clazz + " via " + customProperty);
    +          return fallbackVolumeChooser;
    --- End diff --
   
    Should we not be failing here instead of using the fallback?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113244305
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java ---
    @@ -68,10 +100,107 @@ public String choose(VolumeChooserEnvironment env, String[] options) {
               chooser = last;
             }
           }
    +    }
    +    return chooser;
    +  }
    +
    +  private VolumeChooser getVolumeChooserForNonTable(VolumeChooserEnvironment env, ServerConfigurationFactory localConf) {
    +    VolumeChooser chooser;
    +    final String customProperty = Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".volume.chooser";
    +
    +    if (log.isTraceEnabled()) {
    +      log.trace("Scope: " + env.getScope());
    +      log.trace("Looking up property: " + customProperty);
    +    }
    +
    +    AccumuloConfiguration systemConfiguration = localConf.getConfiguration();
    +    String clazz = systemConfiguration.get(customProperty);
    +    if (null == clazz || clazz.isEmpty()) {
    +      log.debug("Property not found: " + customProperty + " using fallback chooser.");
    +      return fallbackVolumeChooser;
         } else {
    -      chooser = fallbackVolumeChooser;
    +      chooser = scopeSpecificChooser.get(env.getScope());
    +      if (chooser == null) {
    +        VolumeChooser temp;
    +        try {
    +          temp = loadClassForCustomProperty(clazz);
    +        } catch (Exception e) {
    +          log.error("Failed to create instance for " + env.getScope() + " configured to use " + clazz + " via " + customProperty);
    +          return fallbackVolumeChooser;
    --- End diff --
   
    Should we not be failing here instead of falling back?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo issue #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on the issue:

    https://github.com/apache/accumulo/pull/253
 
    Do the test cases hang when run via mvn?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113294170
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java ---
    @@ -18,36 +18,65 @@
     
     import java.util.concurrent.ConcurrentHashMap;
     
    +import org.apache.accumulo.core.client.AccumuloException;
    +import org.apache.accumulo.core.conf.AccumuloConfiguration;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
     import org.apache.accumulo.server.conf.TableConfiguration;
    +import org.apache.accumulo.start.classloader.vfs.AccumuloVFSClassLoader;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
     
     /**
      * A {@link VolumeChooser} that delegates to another volume chooser based on the presence of an experimental table property,
      * {@link Property#TABLE_VOLUME_CHOOSER}. If it isn't found, defaults back to {@link RandomVolumeChooser}.
      */
     public class PerTableVolumeChooser implements VolumeChooser {
    -
    -  private final VolumeChooser fallbackVolumeChooser = new RandomVolumeChooser();
    +  private static final Logger log = LoggerFactory.getLogger(PerTableVolumeChooser.class);
       // TODO Add hint of expected size to construction, see ACCUMULO-3410
       /* Track VolumeChooser instances so they can keep state. */
       private final ConcurrentHashMap<String,VolumeChooser> tableSpecificChooser = new ConcurrentHashMap<>();
    +  private final ConcurrentHashMap<String,VolumeChooser> scopeSpecificChooser = new ConcurrentHashMap<>();
    +  private final RandomVolumeChooser randomChooser = new RandomVolumeChooser();
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
    +  private volatile VolumeChooser fallbackVolumeChooser = null;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    VolumeChooser chooser = null;
    -    if (env.hasTableId()) {
    -      // This local variable is an intentional component of the single-check idiom.
    -      ServerConfigurationFactory localConf = serverConfs;
    -      if (localConf == null) {
    -        // If we're under contention when first getting here we'll throw away some initializations.
    -        localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -        serverConfs = localConf;
    -      }
    -      final TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (log.isTraceEnabled()) {
    +      log.trace("PerTableVolumeChooser.choose");
    +    }
    +    VolumeChooser chooser;
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // Should only get here during Initialize. Configurations are not yet available.
    +      return randomChooser.choose(env, options);
    +    }
    +
    +    ServerConfigurationFactory localConf = loadConf();
    +    lazilyCreateFallbackChooser();
    +    if (env.hasScope()) {
    --- End diff --
   
    `env.hasTableId()` and `env.hasScope()` are mutually exclusive right now. It's possible that could change, but we'd have to think about what that might mean. Currently, the only scope is "logger" and it has no table associated with it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113294271
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java ---
    @@ -21,84 +21,90 @@
     import java.util.ArrayList;
     import java.util.Arrays;
     import java.util.Collections;
    -import java.util.HashMap;
     import java.util.HashSet;
     import java.util.Map;
     import java.util.Set;
    -import java.util.function.Predicate;
     
    +import org.apache.accumulo.core.client.AccumuloException;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.core.volume.Volume;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
    -import org.apache.accumulo.server.conf.TableConfiguration;
     import org.apache.commons.collections.map.LRUMap;
     import org.apache.commons.lang.StringUtils;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     /**
      * A {@link RandomVolumeChooser} that limits its choices from a given set of options to the subset of those options preferred for a particular table. Defaults
    - * to selecting from all of the options presented. Can be customized via the table property {@value #PREFERRED_VOLUMES_CUSTOM_KEY}, which should contain a comma
    + * to selecting from all of the options presented. Can be customized via the table property table.custom.preferredVolumes, which should contain a comma
      * separated list of {@link Volume} URIs. Note that both the property name and the format of its value are specific to this particular implementation.
      */
     public class PreferredVolumeChooser extends RandomVolumeChooser {
       private static final Logger log = LoggerFactory.getLogger(PreferredVolumeChooser.class);
     
    -  /**
    -   * This should match {@link Property#TABLE_ARBITRARY_PROP_PREFIX}
    -   */
    -  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = "table.custom.preferredVolumes";
    -  // TODO ACCUMULO-3417 replace this with the ability to retrieve by String key.
    -  private static final Predicate<String> PREFERRED_VOLUMES_FILTER = key -> PREFERRED_VOLUMES_CUSTOM_KEY.equals(key);
    +  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = Property.TABLE_ARBITRARY_PROP_PREFIX + "preferredVolumes";
     
       @SuppressWarnings("unchecked")
       private final Map<String,Set<String>> parsedPreferredVolumes = Collections.synchronizedMap(new LRUMap(1000));
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    if (!env.hasTableId())
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // this should only happen during initialize
    +      log.warn("No table id or scope, so it's not possible to determine preferred volumes.  Using all volumes.");
           return super.choose(env, options);
    +    }
    +    ServerConfigurationFactory localConf = loadConf();
     
    -    // Get the current table's properties, and find the preferred volumes property
    -    // This local variable is an intentional component of the single-check idiom.
    -    ServerConfigurationFactory localConf = serverConfs;
    -    if (localConf == null) {
    -      // If we're under contention when first getting here we'll throw away some initializations.
    -      localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -      serverConfs = localConf;
    +    String systemPreferredVolumes = localConf.getConfiguration().get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    if (null == systemPreferredVolumes || systemPreferredVolumes.isEmpty()) {
    +      String logMessage = "Default preferred volumes are missing.";
    +      log.debug(logMessage);
    +      throw new AccumuloException(logMessage);
         }
    -    TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    -    final Map<String,String> props = new HashMap<>();
    -    tableConf.getProperties(props, PREFERRED_VOLUMES_FILTER);
    -    if (props.isEmpty()) {
    -      log.warn("No preferred volumes specified. Defaulting to randomly choosing from instance volumes");
    -      return super.choose(env, options);
    +
    +    String volumes = null;
    +    if (env.hasTableId()) {
    +      volumes = localConf.getTableConfiguration(env.getTableId()).get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    } else if (env.hasScope()) {
    +      volumes = localConf.getConfiguration().get(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".preferredVolumes");
    --- End diff --
   
    Yeah, that'd be a nice improvement.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113294827
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java ---
    @@ -21,84 +21,90 @@
     import java.util.ArrayList;
     import java.util.Arrays;
     import java.util.Collections;
    -import java.util.HashMap;
     import java.util.HashSet;
     import java.util.Map;
     import java.util.Set;
    -import java.util.function.Predicate;
     
    +import org.apache.accumulo.core.client.AccumuloException;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.core.volume.Volume;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
    -import org.apache.accumulo.server.conf.TableConfiguration;
     import org.apache.commons.collections.map.LRUMap;
     import org.apache.commons.lang.StringUtils;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     /**
      * A {@link RandomVolumeChooser} that limits its choices from a given set of options to the subset of those options preferred for a particular table. Defaults
    - * to selecting from all of the options presented. Can be customized via the table property {@value #PREFERRED_VOLUMES_CUSTOM_KEY}, which should contain a comma
    + * to selecting from all of the options presented. Can be customized via the table property table.custom.preferredVolumes, which should contain a comma
      * separated list of {@link Volume} URIs. Note that both the property name and the format of its value are specific to this particular implementation.
      */
     public class PreferredVolumeChooser extends RandomVolumeChooser {
       private static final Logger log = LoggerFactory.getLogger(PreferredVolumeChooser.class);
     
    -  /**
    -   * This should match {@link Property#TABLE_ARBITRARY_PROP_PREFIX}
    -   */
    -  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = "table.custom.preferredVolumes";
    -  // TODO ACCUMULO-3417 replace this with the ability to retrieve by String key.
    -  private static final Predicate<String> PREFERRED_VOLUMES_FILTER = key -> PREFERRED_VOLUMES_CUSTOM_KEY.equals(key);
    +  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = Property.TABLE_ARBITRARY_PROP_PREFIX + "preferredVolumes";
     
       @SuppressWarnings("unchecked")
       private final Map<String,Set<String>> parsedPreferredVolumes = Collections.synchronizedMap(new LRUMap(1000));
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    if (!env.hasTableId())
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // this should only happen during initialize
    +      log.warn("No table id or scope, so it's not possible to determine preferred volumes.  Using all volumes.");
           return super.choose(env, options);
    +    }
    +    ServerConfigurationFactory localConf = loadConf();
     
    -    // Get the current table's properties, and find the preferred volumes property
    -    // This local variable is an intentional component of the single-check idiom.
    -    ServerConfigurationFactory localConf = serverConfs;
    -    if (localConf == null) {
    -      // If we're under contention when first getting here we'll throw away some initializations.
    -      localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -      serverConfs = localConf;
    +    String systemPreferredVolumes = localConf.getConfiguration().get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    if (null == systemPreferredVolumes || systemPreferredVolumes.isEmpty()) {
    +      String logMessage = "Default preferred volumes are missing.";
    +      log.debug(logMessage);
    +      throw new AccumuloException(logMessage);
         }
    -    TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    -    final Map<String,String> props = new HashMap<>();
    -    tableConf.getProperties(props, PREFERRED_VOLUMES_FILTER);
    -    if (props.isEmpty()) {
    -      log.warn("No preferred volumes specified. Defaulting to randomly choosing from instance volumes");
    -      return super.choose(env, options);
    +
    +    String volumes = null;
    +    if (env.hasTableId()) {
    +      volumes = localConf.getTableConfiguration(env.getTableId()).get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    } else if (env.hasScope()) {
    +      volumes = localConf.getConfiguration().get(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".preferredVolumes");
    +    }
    +
    +    // if there was an empty or missing property, use the system-wide volumes
    +    if (null == volumes || volumes.isEmpty()) {
    +      if (env.hasTableId()) {
    +        log.warn("Missing property for TableID " + env.getTableId() + " but it should have picked up default volumes.");
    --- End diff --
   
    It's probably a warning, because the current behavior falls back, but I think we'd want to change this so it's a hard fail. It seems it would only happen when there's a misconfiguration (or... without atomic config updates, if one property propagates via ZK faster than the corresponding properties).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113294955
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PreferredVolumeChooser.java ---
    @@ -21,84 +21,90 @@
     import java.util.ArrayList;
     import java.util.Arrays;
     import java.util.Collections;
    -import java.util.HashMap;
     import java.util.HashSet;
     import java.util.Map;
     import java.util.Set;
    -import java.util.function.Predicate;
     
    +import org.apache.accumulo.core.client.AccumuloException;
     import org.apache.accumulo.core.conf.Property;
     import org.apache.accumulo.core.volume.Volume;
     import org.apache.accumulo.server.client.HdfsZooInstance;
     import org.apache.accumulo.server.conf.ServerConfigurationFactory;
    -import org.apache.accumulo.server.conf.TableConfiguration;
     import org.apache.commons.collections.map.LRUMap;
     import org.apache.commons.lang.StringUtils;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
     /**
      * A {@link RandomVolumeChooser} that limits its choices from a given set of options to the subset of those options preferred for a particular table. Defaults
    - * to selecting from all of the options presented. Can be customized via the table property {@value #PREFERRED_VOLUMES_CUSTOM_KEY}, which should contain a comma
    + * to selecting from all of the options presented. Can be customized via the table property table.custom.preferredVolumes, which should contain a comma
      * separated list of {@link Volume} URIs. Note that both the property name and the format of its value are specific to this particular implementation.
      */
     public class PreferredVolumeChooser extends RandomVolumeChooser {
       private static final Logger log = LoggerFactory.getLogger(PreferredVolumeChooser.class);
     
    -  /**
    -   * This should match {@link Property#TABLE_ARBITRARY_PROP_PREFIX}
    -   */
    -  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = "table.custom.preferredVolumes";
    -  // TODO ACCUMULO-3417 replace this with the ability to retrieve by String key.
    -  private static final Predicate<String> PREFERRED_VOLUMES_FILTER = key -> PREFERRED_VOLUMES_CUSTOM_KEY.equals(key);
    +  public static final String PREFERRED_VOLUMES_CUSTOM_KEY = Property.TABLE_ARBITRARY_PROP_PREFIX + "preferredVolumes";
     
       @SuppressWarnings("unchecked")
       private final Map<String,Set<String>> parsedPreferredVolumes = Collections.synchronizedMap(new LRUMap(1000));
       // TODO has to be lazily initialized currently because of the reliance on HdfsZooInstance. see ACCUMULO-3411
       private volatile ServerConfigurationFactory serverConfs;
     
       @Override
    -  public String choose(VolumeChooserEnvironment env, String[] options) {
    -    if (!env.hasTableId())
    +  public String choose(VolumeChooserEnvironment env, String[] options) throws AccumuloException {
    +    if (!env.hasTableId() && !env.hasScope()) {
    +      // this should only happen during initialize
    +      log.warn("No table id or scope, so it's not possible to determine preferred volumes.  Using all volumes.");
           return super.choose(env, options);
    +    }
    +    ServerConfigurationFactory localConf = loadConf();
     
    -    // Get the current table's properties, and find the preferred volumes property
    -    // This local variable is an intentional component of the single-check idiom.
    -    ServerConfigurationFactory localConf = serverConfs;
    -    if (localConf == null) {
    -      // If we're under contention when first getting here we'll throw away some initializations.
    -      localConf = new ServerConfigurationFactory(HdfsZooInstance.getInstance());
    -      serverConfs = localConf;
    +    String systemPreferredVolumes = localConf.getConfiguration().get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    if (null == systemPreferredVolumes || systemPreferredVolumes.isEmpty()) {
    +      String logMessage = "Default preferred volumes are missing.";
    +      log.debug(logMessage);
    +      throw new AccumuloException(logMessage);
         }
    -    TableConfiguration tableConf = localConf.getTableConfiguration(env.getTableId());
    -    final Map<String,String> props = new HashMap<>();
    -    tableConf.getProperties(props, PREFERRED_VOLUMES_FILTER);
    -    if (props.isEmpty()) {
    -      log.warn("No preferred volumes specified. Defaulting to randomly choosing from instance volumes");
    -      return super.choose(env, options);
    +
    +    String volumes = null;
    +    if (env.hasTableId()) {
    +      volumes = localConf.getTableConfiguration(env.getTableId()).get(PREFERRED_VOLUMES_CUSTOM_KEY);
    +    } else if (env.hasScope()) {
    +      volumes = localConf.getConfiguration().get(Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".preferredVolumes");
    +    }
    +
    +    // if there was an empty or missing property, use the system-wide volumes
    +    if (null == volumes || volumes.isEmpty()) {
    +      if (env.hasTableId()) {
    +        log.warn("Missing property for TableID " + env.getTableId() + " but it should have picked up default volumes.");
    +      } else {
    +        log.debug("Missing preferred volumes for scope " + env.getScope() + ". Using default volumes.");
    --- End diff --
   
    This should probably be a hard fail also.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113297447
 
    --- Diff: core/src/main/java/org/apache/accumulo/core/conf/Property.java ---
    @@ -191,7 +191,7 @@
           + "server-internal scheduled tasks"),
       // If you update the default type, be sure to update the default used for initialization failures in VolumeManagerImpl
       @Experimental
    -  GENERAL_VOLUME_CHOOSER("general.volume.chooser", "org.apache.accumulo.server.fs.PerTableVolumeChooser", PropertyType.CLASSNAME,
    +  GENERAL_VOLUME_CHOOSER("general.volume.chooser", "org.apache.accumulo.server.fs.RandomVolumeChooser", PropertyType.CLASSNAME,
    --- End diff --
   
    I'm actually questioning whether this is necessary myself. The original intent of the parent issue was to ban certain volumes from being used entirely (unless explicitly desired). This change was to help provide a sane fall-back scenario, but I'm not sure it's necessary if we change things to be a hard-fail in the case of misconfiguration.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113297684
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java ---
    @@ -68,10 +100,107 @@ public String choose(VolumeChooserEnvironment env, String[] options) {
               chooser = last;
             }
           }
    +    }
    +    return chooser;
    +  }
    +
    +  private VolumeChooser getVolumeChooserForNonTable(VolumeChooserEnvironment env, ServerConfigurationFactory localConf) {
    +    VolumeChooser chooser;
    +    final String customProperty = Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".volume.chooser";
    +
    +    if (log.isTraceEnabled()) {
    +      log.trace("Scope: " + env.getScope());
    +      log.trace("Looking up property: " + customProperty);
    +    }
    +
    +    AccumuloConfiguration systemConfiguration = localConf.getConfiguration();
    +    String clazz = systemConfiguration.get(customProperty);
    +    if (null == clazz || clazz.isEmpty()) {
    +      log.debug("Property not found: " + customProperty + " using fallback chooser.");
    +      return fallbackVolumeChooser;
         } else {
    -      chooser = fallbackVolumeChooser;
    +      chooser = scopeSpecificChooser.get(env.getScope());
    +      if (chooser == null) {
    +        VolumeChooser temp;
    +        try {
    +          temp = loadClassForCustomProperty(clazz);
    +        } catch (Exception e) {
    +          log.error("Failed to create instance for " + env.getScope() + " configured to use " + clazz + " via " + customProperty);
    +          return fallbackVolumeChooser;
    +        }
    +        chooser = scopeSpecificChooser.putIfAbsent(env.getScope(), temp);
    +        if (chooser == null) {
    +          chooser = temp;
    +          // Otherwise, someone else beat us to initializing; use theirs.
    +        }
    +      } else if (!(chooser.getClass().getName().equals(clazz))) {
    +        if (log.isTraceEnabled()) {
    +          log.trace("change detected for scope: " + env.getScope());
    +        }
    +        // the configuration for this scope's chooser has been updated. In the case of failure to instantiate we'll repeat here next call.
    +        // TODO stricter definition of when the updated property is used, ref ACCUMULO-3412
    +        VolumeChooser temp;
    +        try {
    +          temp = loadClassForCustomProperty(clazz);
    +        } catch (Exception e) {
    +          log.error("Failed to create instance for " + env.getScope() + " configured to use " + clazz + " via " + customProperty);
    +          return fallbackVolumeChooser;
    --- End diff --
   
    I think we should fail-hard here instead of the current behavior. Matt's original code was to fall back, but we can simplify this, because I don't think that's necessary.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/253#discussion_r113297767
 
    --- Diff: server/base/src/main/java/org/apache/accumulo/server/fs/PerTableVolumeChooser.java ---
    @@ -68,10 +100,107 @@ public String choose(VolumeChooserEnvironment env, String[] options) {
               chooser = last;
             }
           }
    +    }
    +    return chooser;
    +  }
    +
    +  private VolumeChooser getVolumeChooserForNonTable(VolumeChooserEnvironment env, ServerConfigurationFactory localConf) {
    +    VolumeChooser chooser;
    +    final String customProperty = Property.GENERAL_ARBITRARY_PROP_PREFIX.getKey() + env.getScope() + ".volume.chooser";
    +
    +    if (log.isTraceEnabled()) {
    +      log.trace("Scope: " + env.getScope());
    +      log.trace("Looking up property: " + customProperty);
    +    }
    +
    +    AccumuloConfiguration systemConfiguration = localConf.getConfiguration();
    +    String clazz = systemConfiguration.get(customProperty);
    +    if (null == clazz || clazz.isEmpty()) {
    +      log.debug("Property not found: " + customProperty + " using fallback chooser.");
    +      return fallbackVolumeChooser;
         } else {
    -      chooser = fallbackVolumeChooser;
    +      chooser = scopeSpecificChooser.get(env.getScope());
    +      if (chooser == null) {
    +        VolumeChooser temp;
    +        try {
    +          temp = loadClassForCustomProperty(clazz);
    +        } catch (Exception e) {
    +          log.error("Failed to create instance for " + env.getScope() + " configured to use " + clazz + " via " + customProperty);
    +          return fallbackVolumeChooser;
    --- End diff --
   
    Same comment as above. We can simplify this if we fail-hard.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo issue #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ctubbsii commented on the issue:

    https://github.com/apache/accumulo/pull/253
 
    @ivakegg The test cases hung for me in Eclipse. I didn't try on the command-line yet. I'm still trying to understand what the tests are actually trying to do, and how we should change them if we switch to hard fail.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|

[GitHub] accumulo issue #253: ACCUMULO-4086 Improve volume chooser fallback

ctubbsii
In reply to this post by ctubbsii
Github user ivakegg commented on the issue:

    https://github.com/apache/accumulo/pull/253
 
    @ctubbsii  I am attempting to resolve the issues within this pull request.  I am creating a separate branch under my account.  Shall I subsequently create a pull request from my branch to yours so that you can update this pull request or is there another way to do this?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
1234