Return-Path: X-Original-To: apmail-accumulo-commits-archive@www.apache.org Delivered-To: apmail-accumulo-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 0210A10C3D for ; Thu, 20 Mar 2014 15:25:53 +0000 (UTC) Received: (qmail 64306 invoked by uid 500); 20 Mar 2014 15:25:52 -0000 Delivered-To: apmail-accumulo-commits-archive@accumulo.apache.org Received: (qmail 64254 invoked by uid 500); 20 Mar 2014 15:25:51 -0000 Mailing-List: contact commits-help@accumulo.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@accumulo.apache.org Delivered-To: mailing list commits@accumulo.apache.org Received: (qmail 61343 invoked by uid 99); 20 Mar 2014 15:25:20 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 20 Mar 2014 15:25:20 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id 4D5439861EF; Thu, 20 Mar 2014 15:25:19 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: elserj@apache.org To: commits@accumulo.apache.org Date: Thu, 20 Mar 2014 15:25:37 -0000 Message-Id: <511b23889e174cb5965b0e1dae789e26@git.apache.org> In-Reply-To: <6929a2d892494d1ca55f54f78759a16a@git.apache.org> References: <6929a2d892494d1ca55f54f78759a16a@git.apache.org> X-Mailer: ASF-Git Admin Mailer Subject: [20/20] git commit: ACCUMULO-2061 A few more minor fixes from reviewboard. ACCUMULO-2061 A few more minor fixes from reviewboard. Fixed a PrintInfo comment. Expand the VolumeImpl.isValidPath javadoc and implementation to be more encompassing. Suppress warnings in VolumeIT. Remove implementation of isValidPath from NonConfiguredVolume as it could be misleading. Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/8b8d565b Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/8b8d565b Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/8b8d565b Branch: refs/heads/ACCUMULO-2061 Commit: 8b8d565b79a83ad6789356a18027cc2c0417b7f0 Parents: 45aec91 Author: Josh Elser Authored: Thu Mar 20 11:22:38 2014 -0400 Committer: Josh Elser Committed: Thu Mar 20 11:22:38 2014 -0400 ---------------------------------------------------------------------- .../accumulo/core/file/rfile/PrintInfo.java | 7 +++-- .../core/volume/NonConfiguredVolume.java | 30 ++++++-------------- .../org/apache/accumulo/core/volume/Volume.java | 4 +-- .../apache/accumulo/core/volume/VolumeImpl.java | 16 ++++++++++- .../accumulo/server/fs/VolumeManagerImpl.java | 8 +++--- .../java/org/apache/accumulo/test/VolumeIT.java | 1 + 6 files changed, 36 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/8b8d565b/core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java b/core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java index 7ed8f34..dc54b49 100644 --- a/core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java +++ b/core/src/main/java/org/apache/accumulo/core/file/rfile/PrintInfo.java @@ -52,8 +52,11 @@ public class PrintInfo { @SuppressWarnings("deprecation") AccumuloConfiguration aconf = AccumuloConfiguration.getSiteConfiguration(); - // TODO ACCUMULO-2462 This will only work for RFiles in HDFS when the filesystem is defined in the core-site.xml - // on the classpath if a path, and not a URI, is given + // TODO ACCUMULO-2462 This will only work for RFiles (path only, not URI) in HDFS when the correct filesystem for the given file + // is on Property.INSTANCE_DFS_DIR or, when INSTANCE_DFS_DIR is not defined, is on the default filesystem + // defined in the Hadoop's core-site.xml + // + // A workaround is to always provide a URI to this class FileSystem hadoopFs = VolumeConfiguration.getDefaultVolume(conf, aconf).getFileSystem(); FileSystem localFs = FileSystem.getLocal(conf); Opts opts = new Opts(); http://git-wip-us.apache.org/repos/asf/accumulo/blob/8b8d565b/core/src/main/java/org/apache/accumulo/core/volume/NonConfiguredVolume.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/volume/NonConfiguredVolume.java b/core/src/main/java/org/apache/accumulo/core/volume/NonConfiguredVolume.java index 7dcbd88..3cfd1c2 100644 --- a/core/src/main/java/org/apache/accumulo/core/volume/NonConfiguredVolume.java +++ b/core/src/main/java/org/apache/accumulo/core/volume/NonConfiguredVolume.java @@ -16,25 +16,18 @@ */ package org.apache.accumulo.core.volume; -import java.io.IOException; - import org.apache.accumulo.core.conf.Property; -import org.apache.accumulo.core.util.CachedConfiguration; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; -import org.apache.log4j.Logger; /** - * Volume implementation which represents a Volume for which we have a FileSystem - * but no base path because it is not configured via {@link Property#INSTANCE_VOLUMES} + * Volume implementation which represents a Volume for which we have a FileSystem but no base path because it is not configured via + * {@link Property#INSTANCE_VOLUMES} * - * This is useful to handle volumes that have been removed from accumulo-site.xml but references - * to these volumes have not been updated. This Volume should never be used to create new files, - * only to read existing files. + * This is useful to handle volumes that have been removed from accumulo-site.xml but references to these volumes have not been updated. This Volume should + * never be used to create new files, only to read existing files. */ public class NonConfiguredVolume implements Volume { - private static final Logger log = Logger.getLogger(NonConfiguredVolume.class); - private FileSystem fs; public NonConfiguredVolume(FileSystem fs) { @@ -48,27 +41,22 @@ public class NonConfiguredVolume implements Volume { @Override public String getBasePath() { - throw new UnsupportedOperationException("No base path known because this volume isn't configured in accumulo-site.xml"); + throw new UnsupportedOperationException("No base path known because this Volume isn't configured in accumulo-site.xml"); } @Override public Path prefixChild(Path p) { - throw new UnsupportedOperationException("Cannot prefix path because this volume isn't configured in accumulo-site.xml"); + throw new UnsupportedOperationException("Cannot prefix path because this Volume isn't configured in accumulo-site.xml"); } @Override public Path prefixChild(String p) { - throw new UnsupportedOperationException("Cannot prefix path because this volume isn't configured in accumulo-site.xml"); + throw new UnsupportedOperationException("Cannot prefix path because this Volume isn't configured in accumulo-site.xml"); } @Override public boolean isValidPath(Path p) { - try { - return fs.equals(p.getFileSystem(CachedConfiguration.getInstance())); - } catch (IOException e) { - log.debug("Cannot determine FileSystem from path: " + p, e); - } - return false; + throw new UnsupportedOperationException("Cannot determine if path is valid because this Volume isn't configured in accumulo-site.xml"); } @Override @@ -89,4 +77,4 @@ public class NonConfiguredVolume implements Volume { public int hashCode() { return NonConfiguredVolume.class.hashCode() ^ this.fs.hashCode(); } -} \ No newline at end of file +} http://git-wip-us.apache.org/repos/asf/accumulo/blob/8b8d565b/core/src/main/java/org/apache/accumulo/core/volume/Volume.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/volume/Volume.java b/core/src/main/java/org/apache/accumulo/core/volume/Volume.java index 17b2bf3..58b0ada 100644 --- a/core/src/main/java/org/apache/accumulo/core/volume/Volume.java +++ b/core/src/main/java/org/apache/accumulo/core/volume/Volume.java @@ -50,8 +50,8 @@ public interface Volume { public Path prefixChild(String p); /** - * Determine if the Path is valid on this Volume (contained by the basePath) - * @return True if path is contained within the basePath, false otherwise + * Determine if the Path is valid on this Volume. A Path is valid if it is contained + * in the Volume's FileSystem and is rooted beneath the basePath */ public boolean isValidPath(Path p); } http://git-wip-us.apache.org/repos/asf/accumulo/blob/8b8d565b/core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java ---------------------------------------------------------------------- diff --git a/core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java b/core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java index 55ccfbc..f902c35 100644 --- a/core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java +++ b/core/src/main/java/org/apache/accumulo/core/volume/VolumeImpl.java @@ -20,15 +20,21 @@ import static com.google.common.base.Preconditions.checkNotNull; import java.io.IOException; +import jline.internal.Log; + +import org.apache.accumulo.core.util.CachedConfiguration; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; +import org.apache.log4j.Logger; /** * Basic Volume implementation that contains a FileSystem and a base path * that should be used within that filesystem. */ public class VolumeImpl implements Volume { + private static final Logger log = Logger.getLogger(VolumeImpl.class); + protected final FileSystem fs; protected final String basePath; @@ -67,7 +73,15 @@ public class VolumeImpl implements Volume { public boolean isValidPath(Path p) { checkNotNull(p); - return p.toUri().getPath().startsWith(basePath); + try { + if (fs.equals(p.getFileSystem(CachedConfiguration.getInstance()))) { + return p.toUri().getPath().startsWith(basePath); + } + } catch (IOException e) { + log.warn("Could not determine filesystem from path: " + p); + } + + return false; } @Override http://git-wip-us.apache.org/repos/asf/accumulo/blob/8b8d565b/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 64a6390..6a3140b 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 @@ -314,12 +314,12 @@ public class VolumeManagerImpl implements VolumeManager { // we could also not find a matching one. We should still provide a Volume with the // correct FileSystem even though we don't know what the proper base dir is // e.g. Files on volumes that are now removed - log.debug("Found no configured Volume for the given path: " + path); - - return new NonConfiguredVolume(desiredFs); + log.debug("Found candidate Volumes for Path but none of the Paths are valid on the candidates: " + path); } - log.debug("Could not determine volume for Path '" + path + "' from defined volumes"); + log.debug("Could not determine volume for Path: " + path); + + return new NonConfiguredVolume(desiredFs); } catch (IOException ex) { throw new RuntimeException(ex); } http://git-wip-us.apache.org/repos/asf/accumulo/blob/8b8d565b/test/src/test/java/org/apache/accumulo/test/VolumeIT.java ---------------------------------------------------------------------- diff --git a/test/src/test/java/org/apache/accumulo/test/VolumeIT.java b/test/src/test/java/org/apache/accumulo/test/VolumeIT.java index 6c1da2c..0a7ef3f 100644 --- a/test/src/test/java/org/apache/accumulo/test/VolumeIT.java +++ b/test/src/test/java/org/apache/accumulo/test/VolumeIT.java @@ -102,6 +102,7 @@ public class VolumeIT extends ConfigurableMacIT { FileUtils.deleteQuietly(new File(v1.getParent().toUri())); } + @SuppressWarnings("deprecation") @Override public void configure(MiniAccumuloConfigImpl cfg, Configuration hadoopCoreSite) { // Run MAC on two locations in the local file system