Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
From: keith-turner <git@git.apache.org>
To: dev@accumulo.apache.org
Reply-To: dev@accumulo.apache.org
References: <git-pr-253-accumulo@git.apache.org>
In-Reply-To: <git-pr-253-accumulo@git.apache.org>
Subject: [GitHub] accumulo pull request #253: ACCUMULO-4086 Improve volume chooser fallback
Content-Type: text/plain
Message-Id: <20170425162118.D7BE3DF965@git1-us-west.apache.org>
Date: Tue, 25 Apr 2017 16:21:18 +0000 (UTC)
archived-at: Tue, 25 Apr 2017 16:21:21 -0000

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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---