This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/master by this push:
new 7adc1f1 Fix #1463 Resolve relative delete markers on upgrade (#1558)
7adc1f1 is described below
commit 7adc1f1652607f7e8430b687e5f79b705803233a
Author: Mike Miller <[email protected]>
AuthorDate: Fri Mar 13 16:47:24 2020 -0400
Fix #1463 Resolve relative delete markers on upgrade (#1558)
---
.../apache/accumulo/server/fs/FileTypeTest.java | 2 +
.../accumulo/master/upgrade/Upgrader9to10.java | 72 ++++++++++++++++------
.../accumulo/master/upgrade/Upgrader9to10Test.java | 54 +++++++++++-----
3 files changed, 91 insertions(+), 37 deletions(-)
diff --git
a/server/base/src/test/java/org/apache/accumulo/server/fs/FileTypeTest.java
b/server/base/src/test/java/org/apache/accumulo/server/fs/FileTypeTest.java
index ca52f84..a083d48 100644
--- a/server/base/src/test/java/org/apache/accumulo/server/fs/FileTypeTest.java
+++ b/server/base/src/test/java/org/apache/accumulo/server/fs/FileTypeTest.java
@@ -74,7 +74,9 @@ public class FileTypeTest {
assertNull(FileType.WAL.getVolume(new Path("1.2.3.4/aaa-bbb-ccc-ddd")));
assertNull(FileType.TABLE.getVolume(new Path("../2b/t-001/C00.rf")));
+ assertNull(FileType.TABLE.removeVolume(new Path("../2b/t-001/C00.rf")));
assertNull(FileType.TABLE.getVolume(new Path("/t-001/C00.rf")));
+ assertNull(FileType.TABLE.removeVolume(new Path("/t-001/C00.rf")));
assertEquals(new Path("hdfs://nn1/accumulo"),
FileType.TABLE.getVolume(new
Path("hdfs://nn1/accumulo/tables/2b/t-001/C00.rf")));
diff --git
a/server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
b/server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
index 839bcbc..064f63b 100644
---
a/server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
+++
b/server/master/src/main/java/org/apache/accumulo/master/upgrade/Upgrader9to10.java
@@ -33,6 +33,7 @@ import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.Objects;
import java.util.stream.StreamSupport;
import org.apache.accumulo.core.Constants;
@@ -68,7 +69,6 @@ import org.apache.accumulo.fate.zookeeper.ZooReaderWriter;
import org.apache.accumulo.fate.zookeeper.ZooUtil;
import org.apache.accumulo.fate.zookeeper.ZooUtil.NodeExistsPolicy;
import org.apache.accumulo.fate.zookeeper.ZooUtil.NodeMissingPolicy;
-import org.apache.accumulo.server.ServerConstants;
import org.apache.accumulo.server.ServerContext;
import org.apache.accumulo.server.fs.VolumeManager;
import org.apache.accumulo.server.gc.GcVolumeUtil;
@@ -405,6 +405,8 @@ public class Upgrader9to10 implements Upgrader {
try (BatchWriter writer = c.createBatchWriter(tableName, new
BatchWriterConfig())) {
log.info("looking for candidates in table {}", tableName);
Iterator<String> oldCandidates = getOldCandidates(ctx, tableName);
+ String upgradeProp =
ctx.getConfiguration().get(Property.INSTANCE_VOLUMES_UPGRADE_RELATIVE);
+
while (oldCandidates.hasNext()) {
List<String> deletes = readCandidatesInBatch(oldCandidates);
log.info("found {} deletes to upgrade", deletes.size());
@@ -412,7 +414,8 @@ public class Upgrader9to10 implements Upgrader {
// create new formatted delete
log.trace("upgrading delete entry for {}", olddelete);
- String updatedDel = switchToAllVolumes(olddelete);
+ Path absolutePath = resolveRelativeDelete(olddelete, upgradeProp);
+ String updatedDel = switchToAllVolumes(absolutePath);
writer.addMutation(ServerAmpleImpl.createDeleteMutation(updatedDel));
}
@@ -430,26 +433,30 @@ public class Upgrader9to10 implements Upgrader {
}
}
+ /**
+ * If path of file to delete is a directory, change it to all volumes. See
{@link GcVolumeUtil}.
+ * For example: A directory
"hdfs://localhost:9000/accumulo/tables/5a/t-0005" with volume removed
+ * "tables/5a/t-0005" depth = 3 will be switched to
"agcav:/tables/5a/t-0005". A file
+ * "hdfs://localhost:9000/accumulo/tables/5a/t-0005/A0012.rf" with volume
removed
+ * "tables/5a/t-0005/A0012.rf" depth = 4 will be returned as is.
+ */
@VisibleForTesting
- static String switchToAllVolumes(String olddelete) {
- Path relPath = VolumeManager.FileType.TABLE.removeVolume(new
Path(olddelete));
-
- if (relPath == null) {
- // An old style relative delete marker of the form /<table id>/<tablet
dir>[/<file>]
- relPath = new Path("/" + VolumeManager.FileType.TABLE.getDirectory() +
olddelete);
- Preconditions.checkState(
- olddelete.startsWith("/") && relPath.depth() == 3 || relPath.depth()
== 4,
- "Unrecongnized relative delete marker {}", olddelete);
- }
-
- if (relPath.depth() == 3 &&
!relPath.getName().startsWith(Constants.BULK_PREFIX)) {
- return
GcVolumeUtil.getDeleteTabletOnAllVolumesUri(TableId.of(relPath.getParent().getName()),
- relPath.getName());
+ static String switchToAllVolumes(Path olddelete) {
+ Path pathNoVolume =
Objects.requireNonNull(VolumeManager.FileType.TABLE.removeVolume(olddelete),
+ "Invalid delete marker. No volume in path: " + olddelete);
+
+ // a directory path with volume removed will have a depth of 3 so change
volume to all volumes
+ if (pathNoVolume.depth() == 3 &&
!pathNoVolume.getName().startsWith(Constants.BULK_PREFIX)) {
+ return GcVolumeUtil.getDeleteTabletOnAllVolumesUri(
+ TableId.of(pathNoVolume.getParent().getName()),
pathNoVolume.getName());
} else {
- return olddelete;
+ return olddelete.toString();
}
}
+ /**
+ * Return path of the file from old delete markers
+ */
private Iterator<String> getOldCandidates(ServerContext ctx, String
tableName)
throws TableNotFoundException {
Range range = MetadataSchema.DeletesSection.getRange();
@@ -548,7 +555,6 @@ public class Upgrader9to10 implements Upgrader {
}
Path relPath = resolveRelativePath(metaEntry, key);
Path absPath = new Path(upgradeProperty, relPath);
- // Path absPath = new
Path(Paths.get(upgradeProperty).normalize().toString(), relPath);
if (fs.exists(absPath)) {
log.debug("Changing Tablet File path from {} to {}", metaEntry,
absPath);
Mutation m = new Mutation(key.getRow());
@@ -595,7 +601,6 @@ public class Upgrader9to10 implements Upgrader {
}
Path relPath = resolveRelativePath(metaEntry, key);
Path absPath = new Path(upgradeProperty, relPath);
- // Path absPath = new
Path(Paths.get(upgradeProperty).normalize().toString(), relPath);
if (!fs.exists(absPath)) {
throw new IllegalArgumentException("Tablet file " + relPath + "
not found at " + absPath
+ " using volume: " + upgradeProperty);
@@ -615,7 +620,7 @@ public class Upgrader9to10 implements Upgrader {
* Resolve old-style relative paths, returning Path of everything except
volume and base
*/
private static Path resolveRelativePath(String metadataEntry, Key key) {
- String prefix = ServerConstants.TABLE_DIR + "/";
+ String prefix = VolumeManager.FileType.TABLE.getDirectory() + "/";
if (metadataEntry.startsWith("../")) {
// resolve style "../2a/t-0003/C0004.rf"
return new Path(prefix + metadataEntry.substring(3));
@@ -625,4 +630,31 @@ public class Upgrader9to10 implements Upgrader {
return new Path(prefix + tableId.canonical() + metadataEntry);
}
}
+
+ /**
+ * Resolve old relative delete markers of the form /tableId/tabletDir/[file]
to
+ * UpgradeVolume/tables/tableId/tabletDir/[file]
+ */
+ static Path resolveRelativeDelete(String oldDelete, String upgradeProperty) {
+ Path pathNoVolume = VolumeManager.FileType.TABLE.removeVolume(new
Path(oldDelete));
+ Path pathToCheck = new Path(oldDelete);
+
+ // if the volume was removed properly, the path is absolute so return,
otherwise
+ // it is a relative path so proceed with more checks
+ if (pathNoVolume != null)
+ return pathToCheck;
+
+ // A relative path directory of the form "/tableId/tabletDir" will have
depth == 2
+ // A relative path file of the form "/tableId/tabletDir/file" will have
depth == 3
+ Preconditions.checkState(
+ oldDelete.startsWith("/") && (pathToCheck.depth() == 2 ||
pathToCheck.depth() == 3),
+ "Unrecognized relative delete marker {}", oldDelete);
+
+ // found relative paths so verify the property used to build the absolute
paths
+ if (upgradeProperty == null || upgradeProperty.isBlank()) {
+ throw new IllegalArgumentException(
+ "Missing required property " +
Property.INSTANCE_VOLUMES_UPGRADE_RELATIVE.getKey());
+ }
+ return new Path(upgradeProperty,
VolumeManager.FileType.TABLE.getDirectory() + oldDelete);
+ }
}
diff --git
a/server/master/src/test/java/org/apache/accumulo/master/upgrade/Upgrader9to10Test.java
b/server/master/src/test/java/org/apache/accumulo/master/upgrade/Upgrader9to10Test.java
index 9b78dad..2f65f91 100644
---
a/server/master/src/test/java/org/apache/accumulo/master/upgrade/Upgrader9to10Test.java
+++
b/server/master/src/test/java/org/apache/accumulo/master/upgrade/Upgrader9to10Test.java
@@ -25,6 +25,7 @@ import static org.easymock.EasyMock.expect;
import static org.easymock.EasyMock.expectLastCall;
import static org.easymock.EasyMock.replay;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
@@ -57,33 +58,52 @@ import org.slf4j.LoggerFactory;
public class Upgrader9to10Test {
private static Logger log = LoggerFactory.getLogger(Upgrader9to10Test.class);
+ private static final String VOL_PROP = "hdfs://nn1:8020/accumulo";
+
@Test
- public void testSwitchRelative() {
+ public void testSwitchRelativeDeletes() {
+ Path resolved = Upgrader9to10.resolveRelativeDelete("/5a/t-0005",
VOL_PROP);
+ assertEquals(new Path(VOL_PROP + "/tables/5a/t-0005"), resolved);
assertEquals(GcVolumeUtil.getDeleteTabletOnAllVolumesUri(TableId.of("5a"),
"t-0005"),
- Upgrader9to10.switchToAllVolumes("/5a/t-0005"));
- assertEquals("/5a/" + BULK_PREFIX + "0005",
- Upgrader9to10.switchToAllVolumes("/5a/" + BULK_PREFIX + "0005"));
- assertEquals("/5a/t-0005/F0009.rf",
Upgrader9to10.switchToAllVolumes("/5a/t-0005/F0009.rf"));
+ Upgrader9to10.switchToAllVolumes(resolved));
+
+ resolved = Upgrader9to10.resolveRelativeDelete("/5a/" + BULK_PREFIX +
"0005", VOL_PROP);
+ assertEquals(new Path(VOL_PROP + "/tables/5a/" + BULK_PREFIX + "0005"),
resolved);
+ assertEquals(VOL_PROP + "/tables/5a/" + BULK_PREFIX + "0005",
+ Upgrader9to10.switchToAllVolumes(resolved));
+
+ resolved = Upgrader9to10.resolveRelativeDelete("/5a/t-0005/F0009.rf",
VOL_PROP);
+ assertEquals(new Path(VOL_PROP + "/tables/5a/t-0005/F0009.rf"), resolved);
+ assertEquals(VOL_PROP + "/tables/5a/t-0005/F0009.rf",
+ Upgrader9to10.switchToAllVolumes(resolved));
}
@Test(expected = IllegalStateException.class)
- public void testBadRelativeTooShort() {
- Upgrader9to10.switchToAllVolumes("/5a");
+ public void testBadRelativeDeleteTooShort() {
+ Upgrader9to10.resolveRelativeDelete("/5a", VOL_PROP);
}
@Test(expected = IllegalStateException.class)
- public void testBadRelativeTooLong() {
- Upgrader9to10.switchToAllVolumes("/5a/5a/t-0005/F0009.rf");
+ public void testBadRelativeDeleteTooLong() throws Exception {
+ Upgrader9to10.resolveRelativeDelete("/5a/5a/t-0005/F0009.rf", VOL_PROP);
}
@Test
- public void testSwitch() {
+ public void testSwitchAllVolumes() {
+ Path resolved = Upgrader9to10
+
.resolveRelativeDelete("hdfs://localhost:9000/accumulo/tables/5a/t-0005",
VOL_PROP);
assertEquals(GcVolumeUtil.getDeleteTabletOnAllVolumesUri(TableId.of("5a"),
"t-0005"),
-
Upgrader9to10.switchToAllVolumes("hdfs://localhost:9000/accumulo/tables/5a/t-0005"));
- assertEquals("hdfs://localhost:9000/accumulo/tables/5a/" + BULK_PREFIX +
"0005", Upgrader9to10
- .switchToAllVolumes("hdfs://localhost:9000/accumulo/tables/5a/" +
BULK_PREFIX + "0005"));
- assertEquals("hdfs://localhost:9000/accumulo/tables/5a/t-0005/C0009.rf",
Upgrader9to10
-
.switchToAllVolumes("hdfs://localhost:9000/accumulo/tables/5a/t-0005/C0009.rf"));
+ Upgrader9to10.switchToAllVolumes(resolved));
+
+ resolved = Upgrader9to10.resolveRelativeDelete(
+ "hdfs://localhost:9000/accumulo/tables/5a/" + BULK_PREFIX + "0005",
VOL_PROP);
+ assertEquals("hdfs://localhost:9000/accumulo/tables/5a/" + BULK_PREFIX +
"0005",
+ Upgrader9to10.switchToAllVolumes(resolved));
+
+ resolved = Upgrader9to10.resolveRelativeDelete(
+ "hdfs://localhost:9000/accumulo/tables/5a/t-0005/C0009.rf", VOL_PROP);
+ assertEquals("hdfs://localhost:9000/accumulo/tables/5a/t-0005/C0009.rf",
+ Upgrader9to10.switchToAllVolumes(resolved));
}
@Test
@@ -159,8 +179,8 @@ public class Upgrader9to10Test {
List<Mutation> results = new ArrayList<>();
setupMocks(c, fs, map, results);
- assertEquals("Invalid Relative path check",
- Upgrader9to10.checkForRelativePaths(c, fs, tableName, volumeUpgrade),
false);
+ assertFalse("Invalid Relative path check",
+ Upgrader9to10.checkForRelativePaths(c, fs, tableName, volumeUpgrade));
assertTrue(results.isEmpty());
}
