ctubbsii commented on code in PR #2716:
URL: https://github.com/apache/accumulo/pull/2716#discussion_r875376785
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -51,64 +51,91 @@ public class GarbageCollectionAlgorithm {
private static final Logger log =
LoggerFactory.getLogger(GarbageCollectionAlgorithm.class);
+ /**
+ * This method takes a file or directory path and returns a relative path in
1 of 2 forms:
+ *
+ * <pre>
+ * 1- For files: table-id/tablet-directory/filename.rf
+ * 2- For directories: table-id/tablet-directory
+ * </pre>
+ *
+ * For example, for full file path like
hdfs://foo:6000/accumulo/tables/4/t0/F000.rf it will
+ * return 4/t0/F000.rf. For a directory that already is relative, like 4/t0,
it will just return
+ * the original path. This method will also remove prefixed relative path.
+ */
private String makeRelative(String path, int expectedLen) {
String relPath = path;
+ // remove prefixed old relative path
if (relPath.startsWith("../"))
relPath = relPath.substring(3);
+ // remove trailing slash
while (relPath.endsWith("/"))
relPath = relPath.substring(0, relPath.length() - 1);
+ // remove beginning slash
while (relPath.startsWith("/"))
relPath = relPath.substring(1);
- String[] tokens = relPath.split("/");
-
- // handle paths like a//b///c
- boolean containsEmpty = false;
- for (String token : tokens) {
- if (token.equals("")) {
- containsEmpty = true;
- break;
- }
- }
-
- if (containsEmpty) {
- ArrayList<String> tmp = new ArrayList<>();
- for (String token : tokens) {
- if (!token.equals("")) {
- tmp.add(token);
- }
- }
-
- tokens = tmp.toArray(new String[tmp.size()]);
- }
+ String[] tokens = removeEmptyTokensFromPath(relPath);
Review Comment:
On second thought, this entire method can go away in favor of a relatively
simple pipeline that strips out the empty components directly into an array
result:
```suggestion
String[] tokens =
stream(relPath.split("/")).filter(not(""::equals)).toArray(String[]::new);
```
(`stream()` is a statically imported `Arrays.stream()` and `not` is
`Predicate.not`; static import is optional, but it becomes an easy to read
one-liner if you do)
##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -51,64 +51,91 @@ public class GarbageCollectionAlgorithm {
private static final Logger log =
LoggerFactory.getLogger(GarbageCollectionAlgorithm.class);
+ /**
+ * This method takes a file or directory path and returns a relative path in
1 of 2 forms:
+ *
+ * <pre>
+ * 1- For files: table-id/tablet-directory/filename.rf
+ * 2- For directories: table-id/tablet-directory
+ * </pre>
+ *
+ * For example, for full file path like
hdfs://foo:6000/accumulo/tables/4/t0/F000.rf it will
+ * return 4/t0/F000.rf. For a directory that already is relative, like 4/t0,
it will just return
+ * the original path. This method will also remove prefixed relative path.
+ */
private String makeRelative(String path, int expectedLen) {
String relPath = path;
+ // remove prefixed old relative path
if (relPath.startsWith("../"))
relPath = relPath.substring(3);
+ // remove trailing slash
while (relPath.endsWith("/"))
relPath = relPath.substring(0, relPath.length() - 1);
+ // remove beginning slash
while (relPath.startsWith("/"))
relPath = relPath.substring(1);
- String[] tokens = relPath.split("/");
-
- // handle paths like a//b///c
- boolean containsEmpty = false;
- for (String token : tokens) {
- if (token.equals("")) {
- containsEmpty = true;
- break;
- }
- }
-
- if (containsEmpty) {
- ArrayList<String> tmp = new ArrayList<>();
- for (String token : tokens) {
- if (!token.equals("")) {
- tmp.add(token);
- }
- }
-
- tokens = tmp.toArray(new String[tmp.size()]);
- }
+ String[] tokens = removeEmptyTokensFromPath(relPath);
if (tokens.length > 3 && path.contains(":")) {
+ // full file path like hdfs://foo:6000/accumulo/tables/4/t0/F000.rf
if (tokens[tokens.length - 4].equals(Constants.TABLE_DIR)
&& (expectedLen == 0 || expectedLen == 3)) {
+ // return the last 3 tokens after tables, like 4/t0/F000.rf
relPath = tokens[tokens.length - 3] + "/" + tokens[tokens.length - 2]
+ "/"
+ tokens[tokens.length - 1];
} else if (tokens[tokens.length - 3].equals(Constants.TABLE_DIR)
&& (expectedLen == 0 || expectedLen == 2)) {
+ // return the last 2 tokens after tables, like 4/t0
relPath = tokens[tokens.length - 2] + "/" + tokens[tokens.length - 1];
} else {
- throw new IllegalArgumentException(path);
+ throw new IllegalArgumentException("Failed to make path relative. Bad
reference: " + path);
}
} else if (tokens.length == 3 && (expectedLen == 0 || expectedLen == 3)
&& !path.contains(":")) {
+ // we already have a relative path so return it, like 4/t0/F000.rf
relPath = tokens[0] + "/" + tokens[1] + "/" + tokens[2];
} else if (tokens.length == 2 && (expectedLen == 0 || expectedLen == 2)
&& !path.contains(":")) {
+ // return the last 2 tokens of the relative path, like 4/t0
relPath = tokens[0] + "/" + tokens[1];
} else {
- throw new IllegalArgumentException(path);
+ throw new IllegalArgumentException("Failed to make path relative. Bad
reference: " + path);
}
return relPath;
}
+ /**
+ * Handle paths like a//b///c by dropping the empty tokens.
+ */
+ private String[] removeEmptyTokensFromPath(String relPath) {
+ String[] tokens = relPath.split("/");
+
+ boolean containsEmpty = false;
+ for (String token : tokens) {
+ if (token.equals("")) {
+ containsEmpty = true;
+ break;
+ }
+ }
+
+ if (containsEmpty) {
Review Comment:
This can be shortened quite a bit:
```suggestion
if (Arrays.stream(tokens).anyMatch(""::equals)) {
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
