accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From els...@apache.org
Subject [2/3] git commit: ACCUMULO-2974 Include the table id when constructing an absolute path from a relative.
Date Fri, 04 Jul 2014 04:32:58 GMT
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 <elserj@apache.org>
Authored: Wed Jul 2 17:30:25 2014 -0400
Committer: Josh Elser <elserj@apache.org>
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<String> 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<String> 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();


Mime
View raw message