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 <mmil...@apache.org> 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()); }