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 83D5611F0F for ; Fri, 4 Jul 2014 04:32:58 +0000 (UTC) Received: (qmail 18411 invoked by uid 500); 4 Jul 2014 04:32:58 -0000 Delivered-To: apmail-accumulo-commits-archive@accumulo.apache.org Received: (qmail 18308 invoked by uid 500); 4 Jul 2014 04:32:58 -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 18278 invoked by uid 99); 4 Jul 2014 04:32:58 -0000 Received: from tyr.zones.apache.org (HELO tyr.zones.apache.org) (140.211.11.114) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 04 Jul 2014 04:32:58 +0000 Received: by tyr.zones.apache.org (Postfix, from userid 65534) id CE368999F24; Fri, 4 Jul 2014 04:32:57 +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: Fri, 04 Jul 2014 04:32:58 -0000 Message-Id: <3f2a493761a84bf69342a86ded57805b@git.apache.org> In-Reply-To: References: X-Mailer: ASF-Git Admin Mailer Subject: [2/3] git commit: ACCUMULO-2974 Include the table id when constructing an absolute path from a relative. ACCUMULO-2974 Include the table id when constructing an absolute path from a relative. Testing that the TabletGroupWatcher does the correct path is difficult, and also doesn't prevent other callers from writing the same bug, so the fix is added to VolumeManagerImpl with appropriate tests added to ensure failure happens. Project: http://git-wip-us.apache.org/repos/asf/accumulo/repo Commit: http://git-wip-us.apache.org/repos/asf/accumulo/commit/5eceb10e Tree: http://git-wip-us.apache.org/repos/asf/accumulo/tree/5eceb10e Diff: http://git-wip-us.apache.org/repos/asf/accumulo/diff/5eceb10e Branch: refs/heads/master Commit: 5eceb10e281b61e1f2b8a27a9b1c28746c2f0fc3 Parents: c731fbf Author: Josh Elser Authored: Wed Jul 2 17:30:25 2014 -0400 Committer: Josh Elser Committed: Fri Jul 4 00:03:42 2014 -0400 ---------------------------------------------------------------------- .../accumulo/server/fs/VolumeManagerImpl.java | 26 +++++- .../server/fs/VolumeManagerImplTest.java | 85 ++++++++++++++++++++ .../accumulo/master/TabletGroupWatcher.java | 6 +- 3 files changed, 114 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/accumulo/blob/5eceb10e/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 9ebdef4..2cdd3fe 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 @@ -37,6 +37,7 @@ import org.apache.accumulo.core.conf.DefaultConfiguration; import org.apache.accumulo.core.conf.Property; import org.apache.accumulo.core.data.Key; import org.apache.accumulo.core.data.KeyExtent; +import org.apache.accumulo.core.file.rfile.RFile; import org.apache.accumulo.core.util.CachedConfiguration; import org.apache.accumulo.core.volume.NonConfiguredVolume; import org.apache.accumulo.core.volume.Volume; @@ -55,6 +56,7 @@ import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.DistributedFileSystem; import org.apache.hadoop.util.Progressable; +import org.apache.hadoop.util.StringUtils; import org.apache.log4j.Logger; import com.google.common.collect.HashMultimap; @@ -538,10 +540,30 @@ public class VolumeManagerImpl implements VolumeManager { } } - // normalize the path - Path fullPath = new Path(defaultVolume.getBasePath(), fileType.getDirectory()); if (path.startsWith("/")) path = path.substring(1); + + // ACCUMULO-2974 To ensure that a proper absolute path is created, the caller needs to include the table ID + // in the relative path. Fail when this doesn't appear to happen. + if (FileType.TABLE == fileType) { + // Trailing slash doesn't create an additional element + String[] pathComponents = StringUtils.split(path, Path.SEPARATOR_CHAR); + + // Is an rfile + if (path.endsWith(RFile.EXTENSION)) { + if (pathComponents.length < 3) { + throw new IllegalArgumentException("Fewer components in file path than expected"); + } + } else { + // is a directory + if (pathComponents.length < 2) { + throw new IllegalArgumentException("Fewer components in directory path than expected"); + } + } + } + + // normalize the path + Path fullPath = new Path(defaultVolume.getBasePath(), fileType.getDirectory()); fullPath = new Path(fullPath, path); FileSystem fs = getVolumeByPath(fullPath).getFileSystem(); http://git-wip-us.apache.org/repos/asf/accumulo/blob/5eceb10e/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 new file mode 100644 index 0000000..f29d220 --- /dev/null +++ b/server/base/src/test/java/org/apache/accumulo/server/fs/VolumeManagerImplTest.java @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.accumulo.server.fs; + +import java.util.Arrays; +import java.util.List; + +import org.apache.accumulo.server.fs.VolumeManager.FileType; +import org.apache.hadoop.fs.Path; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +/** + * + */ +public class VolumeManagerImplTest { + + protected VolumeManager fs; + + @Before + public void setup() throws Exception { + fs = VolumeManagerImpl.getLocal(System.getProperty("user.dir")); + } + + @Test(expected = IllegalArgumentException.class) + public void defaultTabletDirWithoutTableId() throws Exception { + fs.getFullPath(FileType.TABLE, "/default_tablet/"); + } + + @Test(expected = IllegalArgumentException.class) + public void tabletDirWithoutTableId() throws Exception { + fs.getFullPath(FileType.TABLE, "/t-0000001/"); + } + + @Test(expected = IllegalArgumentException.class) + public void defaultTabletFileWithoutTableId() throws Exception { + fs.getFullPath(FileType.TABLE, "/default_tablet/C0000001.rf"); + } + + @Test(expected = IllegalArgumentException.class) + public void tabletFileWithoutTableId() throws Exception { + fs.getFullPath(FileType.TABLE, "/t-0000001/C0000001.rf"); + } + + @Test + public void tabletDirWithTableId() throws Exception { + String basePath = fs.getDefaultVolume().getBasePath(); + String scheme = fs.getDefaultVolume().getFileSystem().getUri().toURL().getProtocol(); + System.out.println(basePath); + Path expectedBase = new Path(scheme + ":" + basePath, FileType.TABLE.getDirectory()); + List pathsToTest = Arrays.asList("1/default_tablet", "1/default_tablet/", "1/t-0000001"); + for (String pathToTest : pathsToTest) { + Path fullPath = fs.getFullPath(FileType.TABLE, pathToTest); + Assert.assertEquals(new Path(expectedBase, pathToTest), fullPath); + } + } + + @Test + public void tabletFileWithTableId() throws Exception { + String basePath = fs.getDefaultVolume().getBasePath(); + String scheme = fs.getDefaultVolume().getFileSystem().getUri().toURL().getProtocol(); + System.out.println(basePath); + Path expectedBase = new Path(scheme + ":" + basePath, FileType.TABLE.getDirectory()); + List pathsToTest = Arrays.asList("1/default_tablet/C0000001.rf", "1/t-0000001/C0000001.rf"); + for (String pathToTest : pathsToTest) { + Path fullPath = fs.getFullPath(FileType.TABLE, pathToTest); + Assert.assertEquals(new Path(expectedBase, pathToTest), fullPath); + } + } +} http://git-wip-us.apache.org/repos/asf/accumulo/blob/5eceb10e/server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java ---------------------------------------------------------------------- diff --git a/server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java b/server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java index d72abd2..fbc9738 100644 --- a/server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java +++ b/server/master/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java @@ -80,6 +80,7 @@ import org.apache.accumulo.server.security.SystemCredentials; import org.apache.accumulo.server.tables.TableManager; import org.apache.accumulo.server.tablets.TabletTime; import org.apache.accumulo.server.util.MetadataTableUtil; +import org.apache.hadoop.fs.Path; import org.apache.hadoop.io.Text; import org.apache.thrift.TException; @@ -512,7 +513,10 @@ class TabletGroupWatcher extends Daemon { } else if (key.compareColumnFamily(TabletsSection.CurrentLocationColumnFamily.NAME) == 0) { throw new IllegalStateException("Tablet " + key.getRow() + " is assigned during a merge!"); } else if (TabletsSection.ServerColumnFamily.DIRECTORY_COLUMN.hasColumns(key)) { - datafiles.add(new FileRef(entry.getValue().toString(), this.master.fs.getFullPath(FileType.TABLE, entry.getValue().toString()))); + // ACCUMULO-2974 Need to include the TableID when converting a relative path to an absolute path. + // The value has the leading path separator already included so it doesn't need it included. + datafiles.add(new FileRef(entry.getValue().toString(), this.master.fs.getFullPath(FileType.TABLE, Path.SEPARATOR + extent.getTableId() + + entry.getValue().toString()))); if (datafiles.size() > 1000) { MetadataTableUtil.addDeleteEntries(extent, datafiles, SystemCredentials.get()); datafiles.clear();