ACCUMULO-3400 Add sanity check for VolumeChooser return Backports the sanity check from ACCUMULO-3393 to 1.6. Ensures that VolumeChoosers only choose from the available options, and don't manifest choices of their own to return, which aren't available.
Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/86f7c1b0 Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/86f7c1b0 Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/86f7c1b0 Branch: refs/heads/master Commit: 86f7c1b0866025171a1993abc01ce60ccfca859a Parents: b6f9a1e Author: Christopher Tubbs <ctubb...@apache.org> Authored: Mon Dec 15 14:33:51 2014 -0500 Committer: Christopher Tubbs <ctubb...@apache.org> Committed: Mon Dec 15 14:33:51 2014 -0500 ---------------------------------------------------------------------- .../accumulo/server/fs/VolumeManagerImpl.java | 12 +++++++++- .../server/fs/VolumeManagerImplTest.java | 23 ++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/86f7c1b0/server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java ---------------------------------------------------------------------- diff --git a/server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java b/server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java index 38a8369..912d031 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java +++ b/server/base/src/main/java/org/apache/accumulo/server/fs/VolumeManagerImpl.java @@ -44,6 +44,7 @@ import org.apache.accumulo.core.volume.NonConfiguredVolume; import org.apache.accumulo.core.volume.Volume; import org.apache.accumulo.core.volume.VolumeConfiguration; import org.apache.accumulo.server.conf.ServerConfiguration; +import org.apache.commons.lang.ArrayUtils; import org.apache.commons.lang.NotImplementedException; import org.apache.commons.lang.StringUtils; import org.apache.hadoop.conf.Configuration; @@ -571,9 +572,18 @@ public class VolumeManagerImpl implements VolumeManager { return getVolumeByPath(dir).getFileSystem().getContentSummary(dir); } + // Only used as a fall back if the configured chooser misbehaves. + private final VolumeChooser failsafeChooser = new RandomVolumeChooser(); + @Override public String choose(String[] options) { - return chooser.choose(options); + final String choice = chooser.choose(options); + if (!(ArrayUtils.contains(options, choice))) { + log.error("The configured volume chooser, '" + chooser.getClass() + "', or one of its delegates returned a volume not in the set of options provided; " + + "will continue by relying on a RandomVolumeChooser. You should investigate and correct the named chooser."); + return failsafeChooser.choose(options); + } + return choice; } @Override http://git-wip-us.apache.org/repos/asf/accumulo/blob/86f7c1b0/server/base/src/test/java/org/apache/accumulo/server/fs/VolumeManagerImplTest.java ---------------------------------------------------------------------- diff --git a/server/base/src/test/java/org/apache/accumulo/server/fs/VolumeManagerImplTest.java b/server/base/src/test/java/org/apache/accumulo/server/fs/VolumeManagerImplTest.java index 582822a..26a23a3 100644 --- a/server/base/src/test/java/org/apache/accumulo/server/fs/VolumeManagerImplTest.java +++ b/server/base/src/test/java/org/apache/accumulo/server/fs/VolumeManagerImplTest.java @@ -22,6 +22,7 @@ import java.util.List; import org.apache.accumulo.core.conf.ConfigurationCopy; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.server.fs.VolumeManager.FileType; +import org.apache.commons.lang.StringUtils; import org.apache.hadoop.fs.Path; import org.junit.Assert; import org.junit.Before; @@ -91,4 +92,26 @@ public class VolumeManagerImplTest { conf.set(Property.INSTANCE_VOLUMES, "viewfs://dummy"); VolumeManagerImpl.get(conf); } + + public static class WrongVolumeChooser implements VolumeChooser { + @Override + public String choose(String[] options) { + return "file://totally-not-given/"; + } + } + + @SuppressWarnings("deprecation") + private static final Property INSTANCE_DFS_URI = Property.INSTANCE_DFS_URI; + + @Test + public void chooseFromOptions() throws Exception { + List<String> volumes = Arrays.asList("file://one/", "file://two/", "file://three/"); + ConfigurationCopy conf = new ConfigurationCopy(); + conf.set(INSTANCE_DFS_URI, volumes.get(0)); + conf.set(Property.INSTANCE_VOLUMES, StringUtils.join(volumes,",")); + conf.set(Property.GENERAL_VOLUME_CHOOSER, WrongVolumeChooser.class.getName()); + VolumeManager vm = VolumeManagerImpl.get(conf); + String choice = vm.choose(volumes.toArray(new String[0])); + Assert.assertTrue("shouldn't see invalid options from misbehaving chooser.", volumes.contains(choice)); + } }
