diff --git a/server/base/src/main/java/org/apache/accumulo/server/manager/recovery/RecoveryPath.java b/server/base/src/main/java/org/apache/accumulo/server/manager/recovery/RecoveryPath.java index 893867bad26..208858f284d 100644 --- a/server/base/src/main/java/org/apache/accumulo/server/manager/recovery/RecoveryPath.java +++ b/server/base/src/main/java/org/apache/accumulo/server/manager/recovery/RecoveryPath.java @@ -18,41 +18,86 @@ */ package org.apache.accumulo.server.manager.recovery; +import java.util.ArrayList; +import java.util.List; + import org.apache.accumulo.server.fs.VolumeManager.FileType; -import org.apache.hadoop.fs.Path; public class RecoveryPath { - // given a wal path, transform it to a recovery path - public static Path getRecoveryPath(Path walPath) { - if (walPath.depth() >= 3 && walPath.toUri().getScheme() != null) { - // its a fully qualified path - String uuid = walPath.getName(); - // drop uuid - walPath = walPath.getParent(); - - // expect and drop the server component - if (walPath.getName().equals(FileType.WAL.getDirectory())) { - throw new IllegalArgumentException("Bath path " + walPath + " (missing server component)"); - } - walPath = walPath.getParent(); + private static int getPathStart(String walPath) { + final String schemeTest = "://"; + + if (walPath == null || walPath.isEmpty()) { + throw new IllegalArgumentException("Bad path " + walPath); + } + + // Test for scheme + int schemeEnd = walPath.indexOf(schemeTest); + if (schemeEnd < 0) { + throw new IllegalArgumentException("Bad path " + walPath + " No scheme"); + } - // expect and drop the wal component - if (!walPath.getName().equals(FileType.WAL.getDirectory())) { - throw new IllegalArgumentException( - "Bad path " + walPath + " (missing wal directory component)"); + // Find the start of the path + int pathStart = walPath.indexOf("/", schemeEnd + schemeTest.length()); + if (pathStart < 0) { + // Empty path after authority + throw new IllegalArgumentException("Bad path " + walPath + " No content"); + } + return pathStart; + } + + static String transformToRecoveryPath(String walPath) { + int pathStart = getPathStart(walPath); + + // This replaces the need for Path.getParent calls + String[] segments = walPath.substring(pathStart).split("/"); + // Remove any spaces + List parts = new ArrayList<>(); + for (String s : segments) { + if (!s.isEmpty()) { + parts.add(s); } - walPath = walPath.getParent(); + } - // create new path in recovery directory that is a sibling to the wal directory (same volume), - // without the server component - walPath = new Path(walPath, FileType.RECOVERY.getDirectory()); - walPath = new Path(walPath, uuid); + // checks for minimum correct depth + if (parts.size() < 3) { + throw new IllegalArgumentException("Bad path due to length" + walPath); + } + + String uuid = parts.get(parts.size() - 1); + String serverComponent = parts.get(parts.size() - 2); + String walDir = parts.get(parts.size() - 3); + + if (serverComponent.equals(FileType.WAL.getDirectory())) { + throw new IllegalArgumentException("Bad path " + walPath + " (missing server component)"); + } + + if (!walDir.equals(FileType.WAL.getDirectory())) { + throw new IllegalArgumentException( + "Bad path " + walPath + " (missing wal directory component)"); + } + + String authority = walPath.substring(0, pathStart); + // Handle file scheme like Hadoop Path and drop the slashes + // This converts file:/// to file:/ + if (authority.equals("file://")) { + authority = "file:"; + } - return walPath; + List baseParts = parts.subList(0, parts.size() - 3); + StringBuilder recoveryPath = new StringBuilder(authority); + for (String part : baseParts) { + recoveryPath.append("/").append(part); } + recoveryPath.append("/").append(FileType.RECOVERY.getDirectory()); + recoveryPath.append("/").append(uuid); + + return recoveryPath.toString(); + } - throw new IllegalArgumentException("Bad path " + walPath); + public static String getRecoveryPath(String walPath) { + return transformToRecoveryPath(walPath); } } diff --git a/server/base/src/test/java/org/apache/accumulo/server/manager/recovery/RecoveryPathTest.java b/server/base/src/test/java/org/apache/accumulo/server/manager/recovery/RecoveryPathTest.java new file mode 100644 index 00000000000..13cecd22580 --- /dev/null +++ b/server/base/src/test/java/org/apache/accumulo/server/manager/recovery/RecoveryPathTest.java @@ -0,0 +1,171 @@ +/* + * 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 + * + * https://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.manager.recovery; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import org.apache.accumulo.server.fs.VolumeManager.FileType; +import org.apache.hadoop.fs.Path; +import org.junit.jupiter.api.Test; + +public class RecoveryPathTest { + + private static final String UUID = "2d961760-db4f-47eb-97fe-d283331ec254"; + + private static void assertBothValid(String input, String expectedOutput) { + Path hadoopResult = getRecoveryPath(new Path(input)); + assertEquals(expectedOutput, hadoopResult.toString(), + "getRecoveryPath mismatch for : " + input); + + String stringResult = RecoveryPath.transformToRecoveryPath(input); + assertEquals(expectedOutput, stringResult, "validatePath mismatch for : " + input); + } + + private static void assertBothInvalid(String input, String messageContains) { + var error = assertThrows(IllegalArgumentException.class, () -> getRecoveryPath(new Path(input)), + "getRecoveryPath should throw for: " + input); + assertTrue(error.getMessage().contains(messageContains), "getRecoveryPath message '" + + error.getMessage() + "' does not contain '" + messageContains); + var err = assertThrows(IllegalArgumentException.class, + () -> RecoveryPath.transformToRecoveryPath(input), + "getRecoveryPath should throw for: " + input); + assertTrue(err.getMessage().contains(messageContains), + "validatePath message '" + err.getMessage() + "' does not contain '" + messageContains); + } + + @Test + public void testValidHdfsPathConversions() { + + assertBothValid("hdfs://nn1/accumulo/wal/localhost+9997/" + UUID, + "hdfs://nn1/accumulo/recovery/" + UUID); + + // included port + assertBothValid("hdfs://nn1:9000/accumulo/wal/localhost+9997/" + UUID, + "hdfs://nn1:9000/accumulo/recovery/" + UUID); + + // IP + assertBothValid("hdfs://192.168.1.1:9000/accumulo/wal/192.168.1.2+9997/" + UUID, + "hdfs://192.168.1.1:9000/accumulo/recovery/" + UUID); + + // alternate server hostname + assertBothValid("hdfs://namenode/accumulo/wal/my-host.example.com+9997/" + UUID, + "hdfs://namenode/accumulo/recovery/" + UUID); + + // viewfs vs hdfs + assertBothValid("viewfs://clusterX/accumulo/wal/localhost+9997/" + UUID, + "viewfs://clusterX/accumulo/recovery/" + UUID); + + // Deep basePath + assertBothValid("hdfs://nn1/a/b/c/wal/localhost+9997/" + UUID, + "hdfs://nn1/a/b/c/recovery/" + UUID); + + } + + @Test + public void testValidFilePathConversions() { + assertBothValid("file:///tmp/accumulo/wal/localhost+9997/" + UUID, + "file:/tmp/accumulo/recovery/" + UUID); + } + + @Test + public void testInvalidPaths() { + // No scheme and relative + assertBothInvalid("accumulo/wal/localhost+9997/" + UUID, "Bad path"); + + // No scheme and absolute + assertBothInvalid("/accumulo/wal/localhost+9997/" + UUID, "Bad path"); + + // Only contains uuid under authority + assertBothInvalid("hdfs://nn1/" + UUID, "Bad path"); + // min depth less than 3 + assertBothInvalid("hdfs://nn1/wal/" + UUID, "Bad path"); + + assertBothInvalid("hdfs://nn1/accumulo/wal/" + UUID, "missing server component"); + + assertBothInvalid("hdfs://nn1/accumulo/recovery/localhost+9997/" + UUID, + "missing wal directory component"); + + // Invalid WAL dir case + assertBothInvalid("hdfs://nn1/accumulo/WAL/localhost+9997/" + UUID, + "missing wal directory component"); + } + + @Test + public void testVerifyResultsAreIdentical() { + String[] validPaths = {"hdfs://nn1:9000/accumulo/wal/localhost+9997/" + UUID, + "file:///tmp/acc/wal/host+9997/" + UUID, "hdfs://nn1/a/b/c/wal/s+1/" + UUID, + "viewfs://c/base/wal/h+2/" + UUID,}; + + for (String path : validPaths) { + String hadoopResult = getRecoveryPath(new Path(path)).toString(); + String stringResult = RecoveryPath.transformToRecoveryPath(path); + assertEquals(hadoopResult, stringResult, "Results diverge for path: " + path); + } + } + + @Test + public void testRejectionsAreIdentical() { + String[] invalidPaths = {"/accumulo/wal/localhost+9997/" + UUID, // no scheme + "hdfs://nn1/wal/" + UUID, // too shallow + "hdfs://nn1/acc/tables/h+1/" + UUID, // wrong directory + }; + + for (String path : invalidPaths) { + assertThrows(IllegalArgumentException.class, () -> getRecoveryPath(new Path(path)), + "getRecoveryPath should reject: " + path); + assertThrows(IllegalArgumentException.class, () -> RecoveryPath.transformToRecoveryPath(path), + "validatePath should reject: " + path); + } + } + + // given a wal path, transform it to a recovery path + static Path getRecoveryPath(Path walPath) { + if (walPath.depth() >= 3 && walPath.toUri().getScheme() != null) { + // it's a fully qualified path + String uuid = walPath.getName(); + // drop uuid + walPath = walPath.getParent(); + + // expect and drop the server component + if (walPath.getName().equals(FileType.WAL.getDirectory())) { + throw new IllegalArgumentException("Bath path " + walPath + " (missing server component)"); + } + walPath = walPath.getParent(); + + // expect and drop the wal component + if (!walPath.getName().equals(FileType.WAL.getDirectory())) { + throw new IllegalArgumentException( + "Bad path " + walPath + " (missing wal directory component)"); + } + walPath = walPath.getParent(); + + // create new path in recovery directory that is a sibling to the wal directory (same volume), + // without the server component + walPath = new Path(walPath, FileType.RECOVERY.getDirectory()); + walPath = new Path(walPath, uuid); + + return walPath; + } + + throw new IllegalArgumentException("Bad path " + walPath); + } + +} diff --git a/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java b/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java index 8e1e00988f6..bfd1a2cd419 100644 --- a/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java +++ b/server/manager/src/main/java/org/apache/accumulo/manager/recovery/RecoveryManager.java @@ -111,9 +111,9 @@ public void run() { initiateSort(sortId, source, destination); } } catch (FileNotFoundException e) { - log.debug("Unable to initiate log sort for " + source + ": " + e); + log.debug("Unable to initiate log sort for {}: {}", source, e); } catch (Exception e) { - log.warn("Failed to initiate log sort " + source, e); + log.warn("Failed to initiate log sort {}", source, e); } finally { if (!rescheduled) { synchronized (RecoveryManager.this) { @@ -135,7 +135,7 @@ private void initiateSort(String sortId, String source, final String destination sortsQueued.add(sortId); } - log.info("Created zookeeper entry {} with data {}", Constants.ZRECOVERY + "/" + sortId, work); + log.info("Created zookeeper entry {}{}{} with data {}", Constants.ZRECOVERY, "/", sortId, work); } private boolean exists(final Path path) throws IOException { @@ -192,7 +192,7 @@ private boolean recoverLog(LogEntry walog) throws IOException { String sortId = walog.getUniqueID().toString(); String filename = walog.getPath(); - String dest = RecoveryPath.getRecoveryPath(new Path(filename)).toString(); + String dest = RecoveryPath.getRecoveryPath(filename); boolean sortQueued; synchronized (this) { diff --git a/server/tserver/src/main/java/org/apache/accumulo/tserver/log/ResolvedSortedLog.java b/server/tserver/src/main/java/org/apache/accumulo/tserver/log/ResolvedSortedLog.java index b9faa8c8f76..e13498e5220 100644 --- a/server/tserver/src/main/java/org/apache/accumulo/tserver/log/ResolvedSortedLog.java +++ b/server/tserver/src/main/java/org/apache/accumulo/tserver/log/ResolvedSortedLog.java @@ -87,7 +87,7 @@ public static ResolvedSortedLog resolve(LogEntry logEntry, VolumeManager fs) thr // convert the path of on unsorted logs to the expected path for the corresponding sorted log // dir - Path sortedLogPath = RecoveryPath.getRecoveryPath(new Path(logEntry.getPath())); + Path sortedLogPath = new Path(RecoveryPath.getRecoveryPath(logEntry.getPath())); boolean foundFinish = false; // Path::getName compares the last component of each Path value. In this case, the last diff --git a/test/src/main/java/org/apache/accumulo/test/RecoveryIT.java b/test/src/main/java/org/apache/accumulo/test/RecoveryIT.java index 07133ff3080..4f2c31481c5 100644 --- a/test/src/main/java/org/apache/accumulo/test/RecoveryIT.java +++ b/test/src/main/java/org/apache/accumulo/test/RecoveryIT.java @@ -55,7 +55,6 @@ import org.apache.accumulo.test.functional.ReadWriteIT; import org.apache.accumulo.test.util.Wait; import org.apache.hadoop.conf.Configuration; -import org.apache.hadoop.fs.Path; import org.apache.hadoop.fs.RawLocalFileSystem; import org.apache.hadoop.io.Text; import org.junit.jupiter.api.Tag; @@ -226,7 +225,7 @@ private boolean logSortingCompleted(AccumuloClient c, TableId tableId) throws Ex for (LogEntry walog : tm.getLogs()) { String sortId = walog.getUniqueID().toString(); String filename = walog.getPath(); - String dest = RecoveryPath.getRecoveryPath(new Path(filename)).toString(); + String dest = RecoveryPath.getRecoveryPath(filename); if (ctx.getZooCache().get(Constants.ZRECOVERY + "/" + sortId) != null || !ctx.getVolumeManager().exists(SortedLogState.getFinishedMarkerPath(dest))) {