ctubbsii commented on code in PR #5478:
URL: https://github.com/apache/accumulo/pull/5478#discussion_r2045605419
##########
core/src/main/java/org/apache/accumulo/core/file/FilePrefix.java:
##########
@@ -18,25 +18,90 @@
*/
package org.apache.accumulo.core.file;
-import java.util.stream.Stream;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+import com.google.common.base.Preconditions;
public enum FilePrefix {
- BULK_IMPORT("I"), MINOR_COMPACTION("F"), MAJOR_COMPACTION("C"),
MAJOR_COMPACTION_ALL_FILES("A");
+ ALL("*"),
+ MINOR_COMPACTION("F"),
+ BULK_IMPORT("I"),
+ MAJOR_COMPACTION("C"),
+ MAJOR_COMPACTION_ALL_FILES("A"),
+ MERGING_MINOR_COMPACTION("M");
Review Comment:
Didn't we get rid of merging minor compactions?
##########
core/src/main/java/org/apache/accumulo/core/file/FilePrefix.java:
##########
@@ -18,25 +18,90 @@
*/
package org.apache.accumulo.core.file;
-import java.util.stream.Stream;
+import java.util.EnumSet;
+import java.util.HashSet;
+import java.util.Objects;
+import java.util.Set;
+
+import com.google.common.base.Preconditions;
public enum FilePrefix {
- BULK_IMPORT("I"), MINOR_COMPACTION("F"), MAJOR_COMPACTION("C"),
MAJOR_COMPACTION_ALL_FILES("A");
+ ALL("*"),
+ MINOR_COMPACTION("F"),
+ BULK_IMPORT("I"),
+ MAJOR_COMPACTION("C"),
+ MAJOR_COMPACTION_ALL_FILES("A"),
+ MERGING_MINOR_COMPACTION("M");
final String prefix;
FilePrefix(String prefix) {
this.prefix = prefix;
}
+ public String toPrefix() {
+ return this.prefix;
+ }
+
+ public String createFileName(String fileName) {
+ Objects.requireNonNull(fileName, "filename must be supplied");
+ Preconditions.checkArgument(!fileName.isBlank(), "Empty filename
supplied");
+ if (this == ALL || this == MERGING_MINOR_COMPACTION) {
+ throw new IllegalStateException(
+ "Unable to create filename with ALL, MERGING_MINOR_COMPACTION, or
UNKNOWN prefix");
+ }
+ return prefix + fileName;
+ }
+
public static FilePrefix fromPrefix(String prefix) {
- return Stream.of(FilePrefix.values()).filter(p ->
p.prefix.equals(prefix)).findAny()
- .orElseThrow(() -> new IllegalArgumentException("Unknown prefix type:
" + prefix));
+ Objects.requireNonNull(prefix, "prefix must be supplied");
+ Preconditions.checkArgument(!prefix.isBlank(), "Empty prefix supplied");
+ Preconditions.checkArgument(prefix.length() == 1, "Invalid prefix
supplied: " + prefix);
+ switch (prefix.toUpperCase()) {
+ case "A":
+ return MAJOR_COMPACTION_ALL_FILES;
+ case "C":
+ return MAJOR_COMPACTION;
+ case "F":
+ return MINOR_COMPACTION;
+ case "I":
+ return BULK_IMPORT;
+ case "M":
+ return MERGING_MINOR_COMPACTION;
+ default:
+ throw new IllegalArgumentException("Unknown prefix type: " + prefix);
Review Comment:
You changed this check so it's no longer case-sensitive. Why? If there's a
typo in the prefix, I think we would want to cause an error, not allow it to go
through silently... possibly returning a type that was not intended.
Also, the old code is much more future-proof, and we don't need to maintain
a case statement each time the enum set changes. I don't think the fixed-size
stream over the enum values array is performance-critical, and should be fine.
##########
core/src/test/java/org/apache/accumulo/core/file/FilePrefixTest.java:
##########
@@ -18,22 +18,79 @@
*/
package org.apache.accumulo.core.file;
+import static org.apache.accumulo.core.file.FilePrefix.ALL;
+import static org.apache.accumulo.core.file.FilePrefix.BULK_IMPORT;
+import static org.apache.accumulo.core.file.FilePrefix.MAJOR_COMPACTION;
+import static
org.apache.accumulo.core.file.FilePrefix.MAJOR_COMPACTION_ALL_FILES;
+import static
org.apache.accumulo.core.file.FilePrefix.MERGING_MINOR_COMPACTION;
+import static org.apache.accumulo.core.file.FilePrefix.MINOR_COMPACTION;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
+import java.util.EnumSet;
+
import org.junit.jupiter.api.Test;
public class FilePrefixTest {
@Test
- public void testPrefixes() {
- assertEquals(FilePrefix.BULK_IMPORT, FilePrefix.fromPrefix("I"));
- assertEquals(FilePrefix.MINOR_COMPACTION, FilePrefix.fromPrefix("F"));
- assertEquals(FilePrefix.MAJOR_COMPACTION, FilePrefix.fromPrefix("C"));
- assertEquals(FilePrefix.MAJOR_COMPACTION_ALL_FILES,
FilePrefix.fromPrefix("A"));
+ public void testFromPrefix() {
+ assertThrows(NullPointerException.class, () ->
FilePrefix.fromPrefix(null));
+ assertThrows(IllegalArgumentException.class, () ->
FilePrefix.fromPrefix(""));
+ assertThrows(IllegalArgumentException.class, () ->
FilePrefix.fromPrefix("AB"));
+ assertEquals(MAJOR_COMPACTION, FilePrefix.fromPrefix("c"));
+ assertEquals(MAJOR_COMPACTION, FilePrefix.fromPrefix("C"));
Review Comment:
You could simplify similar test cases:
```suggestion
Set.of("c", "C").forEach(s -> assertEquals(MAJOR_COMPACTION,
FilePrefix.fromPrefix(s), "wrong type for prefix " + s));
```
--
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]
