kevinrr888 commented on code in PR #5547:
URL: https://github.com/apache/accumulo/pull/5547#discussion_r2116121170
##########
core/src/main/java/org/apache/accumulo/core/spi/fs/PreferredVolumeChooser.java:
##########
@@ -231,7 +231,10 @@ private Set<String> parsePreferred(String property, String
preferredVolumes,
// preferred volumes should also exist in the original options (typically,
from
// instance.volumes)
if (Collections.disjoint(preferred, options)) {
- String msg = "Some volumes in " + preferred + " are not valid volumes
from " + options;
+ String offendingVols = preferred.stream().filter(vol ->
!options.contains(vol))
+ .collect(Collectors.joining(", "));
+ String msg = "Some volumes in " + preferred + " are not valid volumes
from " + options + ". "
Review Comment:
```suggestion
String msg = "Some volumes in " + preferred + " are not valid volumes
from " + options + ": "
```
##########
core/src/test/java/org/apache/accumulo/core/classloader/ContextClassLoaderFactoryTest.java:
##########
@@ -39,27 +40,33 @@
public class ContextClassLoaderFactoryTest extends WithTestNames {
@TempDir
- private static File tempFolder;
+ private static Path tempFolder;
- private String uri1;
- private String uri2;
+ private URL uri1;
+ private URL uri2;
Review Comment:
This is a test so doesn't really matter but could these be URI instead of URL
##########
core/src/test/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParserTest.java:
##########
@@ -112,19 +111,15 @@ public void testShellOutput() throws Exception {
testShellOutput(configFile -> {
try {
- final Map<String,String> contents = ClusterConfigParser
-
.parseConfiguration(Path.of(configFile.toURI()).toFile().getAbsolutePath());
+ final Map<String,String> contents =
+
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
- final File outputFile =
-
tempDir.toPath().resolve("ClusterConfigParserTest_testShellOutput").toFile();
- if (!outputFile.createNewFile()) {
- fail("Unable to create file in " + tempDir);
- }
- outputFile.deleteOnExit();
Review Comment:
I think this delete on exit may still be needed
##########
core/src/test/java/org/apache/accumulo/core/file/BloomFilterLayerLookupTest.java:
##########
@@ -56,7 +55,7 @@ public class BloomFilterLayerLookupTest extends WithTestNames
{
private static final Logger log =
LoggerFactory.getLogger(BloomFilterLayerLookupTest.class);
@TempDir
- private static File tempDir;
+ private static java.nio.file.Path tempDir;
Review Comment:
```suggestion
private static Path tempDir;
```
Should import
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapred/AccumuloFileOutputFormatIT.java:
##########
@@ -175,38 +176,37 @@ public int run(String[] args) throws Exception {
public static void main(String[] args) throws Exception {
Configuration conf = new Configuration();
conf.set("mapreduce.framework.name", "local");
- conf.set("mapreduce.cluster.local.dir",
java.nio.file.Path.of(System.getProperty("user.dir"))
-
.resolve("target").resolve("mapreduce-tmp").toFile().getAbsolutePath());
+ conf.set("mapreduce.cluster.local.dir",
+ tempDir.resolve("mapreduce-tmp").toAbsolutePath().toString());
assertEquals(0, ToolRunner.run(conf, new MRTester(), args));
}
}
private void handleWriteTests(boolean content) throws Exception {
- File f = tempDir.toPath().resolve(testName()).toFile();
- assertTrue(f.createNewFile(), "Failed to create file: " + f);
- if (f.delete()) {
- log.debug("Deleted {}", f);
- }
- MRTester.main(new String[] {content ? TEST_TABLE : EMPTY_TABLE,
f.getAbsolutePath()});
-
- assertTrue(f.exists());
- File[] files = f.listFiles(file -> file.getName().startsWith("part-m-"));
- assertNotNull(files);
- if (content) {
- assertEquals(1, files.length);
- assertTrue(files[0].exists());
+ java.nio.file.Path f = tempDir.resolve(testName());
+ Files.createFile(f);
+ Files.deleteIfExists(f);
Review Comment:
It seems like this was the functionality before, but this creates a file
then immediately deletes it?
##########
hadoop-mapreduce/src/main/java/org/apache/accumulo/hadoopImpl/mapreduce/lib/DistributedCacheHelper.java:
##########
@@ -58,15 +57,14 @@ public static void addCacheFile(Job job, String path,
String fragment) {
public static InputStream openCachedFile(String path, String fragment,
Configuration conf) {
// try to get it from the local copy provided by the distributed cache
java.nio.file.Path p = java.nio.file.Path.of(fragment);
Review Comment:
```suggestion
Path p = Path.of(fragment);
```
should import
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapred/AccumuloFileOutputFormatIT.java:
##########
@@ -221,12 +221,10 @@ public void writeBadVisibility() throws Exception {
m.put("cf1", "cq2", "A&");
bw.addMutation(m);
bw.close();
- File f = tempDir.toPath().resolve(testName()).toFile();
- assertTrue(f.createNewFile(), "Failed to create file: " + f);
- if (f.delete()) {
- log.debug("Deleted {}", f);
- }
- MRTester.main(new String[] {BAD_TABLE, f.getAbsolutePath()});
+ java.nio.file.Path f = tempDir.resolve(testName());
+ Files.createFile(f);
+ Files.deleteIfExists(f);
Review Comment:
Again, creating then deleting?
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapreduce/TokenFileIT.java:
##########
@@ -155,13 +156,13 @@ public void testMR() throws Exception {
}
}
- File tf = tempDir.toPath().resolve("client.properties").toFile();
- assertTrue(tf.createNewFile(), "Failed to create file: " + tf);
- try (PrintStream out = new PrintStream(tf)) {
+ Path tf = tempDir.resolve("client.properties");
+ Files.createFile(tf);
Review Comment:
missing assertion to check that the file was created
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java:
##########
@@ -130,7 +132,7 @@ public class MiniAccumuloConfigImpl {
* @param rootPassword The initial password for the Accumulo root user
*/
public MiniAccumuloConfigImpl(File dir, String rootPassword) {
- this.dir = dir;
+ this.dir = dir.toPath();
Review Comment:
Would it make more sense to leave dir as File to avoid File -> Path here?
Or maybe can change the arg to Path if there are conversions being done when
this is called
##########
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterClasspathTest.java:
##########
@@ -65,14 +66,15 @@ public class MiniAccumuloClusterClasspathTest extends
WithTestNames {
@BeforeAll
public static void setupMiniCluster() throws Exception {
- File baseDir =
-
Path.of(System.getProperty("user.dir")).resolve("target").resolve("mini-tests").toFile();
- assertTrue(baseDir.mkdirs() || baseDir.isDirectory());
- testDir =
baseDir.toPath().resolve(MiniAccumuloClusterTest.class.getName()).toFile();
+ Path baseDir =
Path.of(System.getProperty("user.dir")).resolve("target").resolve("mini-tests");
+ if (!Files.isDirectory(baseDir)) {
+ Files.createDirectories(baseDir);
+ }
+ testDir =
baseDir.resolve(MiniAccumuloClusterTest.class.getName()).toFile();
Review Comment:
Could testDir be changed to Path?
##########
server/tserver/src/test/java/org/apache/accumulo/tserver/InMemoryMapTest.java:
##########
@@ -800,12 +801,14 @@ IteratorScope.scan,
getServerContext().getTableConfiguration(TableId.of("foo")),
() -> finalIter.seek(new Range(), Set.of(), false));
}
- private String[] uniqueDirPaths(int numOfDirs) {
+ private String[] uniqueDirPaths(int numOfDirs) throws IOException {
String[] newDirs = new String[numOfDirs];
for (int i = 0; i < newDirs.length; i++) {
- File newDir = tempDir.toPath().resolve(testName() + i).toFile();
- assertTrue(newDir.isDirectory() || newDir.mkdir(), "Failed to create
directory: " + newDir);
- newDirs[i] = newDir.getAbsolutePath();
+ Path newDir = tempDir.resolve(testName() + i);
+ if (!Files.isDirectory(newDir)) {
+ Files.createDirectories(newDir);
+ }
+ newDirs[i] = newDir.toAbsolutePath().toString();
Review Comment:
would be good to keep a similar assertion
##########
server/tserver/src/test/java/org/apache/accumulo/tserver/log/TestUpgradePathForWALogs.java:
##########
@@ -64,21 +63,22 @@ public class TestUpgradePathForWALogs extends WithTestNames
{
private TabletServer server;
@TempDir
- private static File tempDir;
+ private static java.nio.file.Path tempDir;
- private static File perTestTempSubDir;
+ private static java.nio.file.Path perTestTempSubDir;
@BeforeEach
public void setUp() throws Exception {
context = createMock(ServerContext.class);
server = createMock(TabletServer.class);
// Create a new subdirectory for each test
- perTestTempSubDir = tempDir.toPath().resolve(testName()).toFile();
- assertTrue(perTestTempSubDir.isDirectory() || perTestTempSubDir.mkdir(),
- "Failed to create folder: " + perTestTempSubDir);
+ perTestTempSubDir = tempDir.resolve(testName());
+ if (!Files.isDirectory(perTestTempSubDir)) {
+ Files.createDirectories(perTestTempSubDir);
+ }
Review Comment:
would be good to keep similar assertion
##########
core/src/test/java/org/apache/accumulo/core/conf/HadoopCredentialProviderTest.java:
##########
@@ -46,6 +48,9 @@
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "paths not
set by user input")
public class HadoopCredentialProviderTest {
+ @TempDir
+ private static java.nio.file.Path tempDir;
Review Comment:
```suggestion
private static Path tempDir;
```
Should import Path
##########
core/src/test/java/org/apache/accumulo/core/file/rfile/GenerateSplitsTest.java:
##########
@@ -76,17 +75,17 @@ public static void createFile() throws IOException {
trf.writer.append(newKey("r6", "cf4", "cq1", "L1", 55), newValue("foo6"));
trf.closeWriter();
- File file = tempDir.toPath().resolve("testGenerateSplits.rf").toFile();
- assertTrue(file.createNewFile(), "Failed to create file: " + file);
- try (var fileOutputStream = Files.newOutputStream(file.toPath())) {
+ Path file = Files.createFile(tempDir.resolve("testGenerateSplits.rf"));
+ assertTrue(Files.exists(file), "Failed to create file: " + file);
+ try (var fileOutputStream = Files.newOutputStream(file)) {
fileOutputStream.write(trf.baos.toByteArray());
}
- rfilePath = "file:" + file.getAbsolutePath();
+ rfilePath = "file:" + file.toAbsolutePath();
log.info("Wrote to file {}", rfilePath);
- File splitsFile = tempDir.toPath().resolve("testSplitsFile").toFile();
- assertTrue(splitsFile.createNewFile(), "Failed to create file: " +
splitsFile);
- splitsFilePath = splitsFile.getAbsolutePath();
+ Path splitsFile = Files.createFile(tempDir.resolve("testSplitsFile"));
+ assertTrue(Files.exists(splitsFile), "Failed to create file: " + file);
Review Comment:
```suggestion
assertTrue(Files.exists(splitsFile), "Failed to create file: " +
splitsFile);
```
##########
core/src/test/java/org/apache/accumulo/core/conf/cluster/ClusterConfigParserTest.java:
##########
@@ -112,19 +111,15 @@ public void testShellOutput() throws Exception {
testShellOutput(configFile -> {
try {
- final Map<String,String> contents = ClusterConfigParser
-
.parseConfiguration(Path.of(configFile.toURI()).toFile().getAbsolutePath());
+ final Map<String,String> contents =
+
ClusterConfigParser.parseConfiguration(Path.of(configFile.toURI()));
- final File outputFile =
-
tempDir.toPath().resolve("ClusterConfigParserTest_testShellOutput").toFile();
- if (!outputFile.createNewFile()) {
- fail("Unable to create file in " + tempDir);
- }
- outputFile.deleteOnExit();
+ final Path outputFile = tempDir.resolve(testName());
+ Files.createFile(outputFile);
Review Comment:
Will this still fail if the file can't be created
##########
core/src/test/java/org/apache/accumulo/core/conf/HadoopCredentialProviderTest.java:
##########
@@ -150,16 +155,10 @@ public void testConfigurationCreation() {
@Test
public void createKeystoreProvider() throws Exception {
- File targetDir =
-
java.nio.file.Path.of(System.getProperty("user.dir")).resolve("target").toFile();
- File keystoreFile = targetDir.toPath().resolve("create.jks").toFile();
- if (keystoreFile.exists()) {
- if (!keystoreFile.delete()) {
- log.error("Unable to delete {}", keystoreFile);
- }
- }
+ java.nio.file.Path keystoreFile = tempDir.resolve("create.jks");
Review Comment:
```suggestion
Path keystorePath = tempDir.resolve("create.jks");
```
Name change not needed, both are correct
##########
core/src/test/java/org/apache/accumulo/core/file/rfile/GenerateSplitsTest.java:
##########
@@ -182,17 +182,17 @@ public void testNullValues() throws Exception {
trf.writer.append(newKey("r6\0f", "cf4", "cq1", "L1", 55),
newValue("foo6"));
trf.closeWriter();
- File file =
tempDir.toPath().resolve("testGenerateSplitsWithNulls.rf").toFile();
- assertTrue(file.createNewFile(), "Failed to create file: " + file);
- try (var fileOutputStream = Files.newOutputStream(file.toPath())) {
+ Path file =
Files.createFile(tempDir.resolve("testGenerateSplitsWithNulls.rf"));
+ assertTrue(Files.exists(file), "Failed to create file: " + file);
+ try (var fileOutputStream = Files.newOutputStream(file)) {
fileOutputStream.write(trf.baos.toByteArray());
}
- rfilePath = "file:" + file.getAbsolutePath();
+ rfilePath = "file:" + file.toAbsolutePath();
log.info("Wrote to file {}", rfilePath);
- File splitsFile =
tempDir.toPath().resolve("testSplitsFileWithNulls").toFile();
- assertTrue(splitsFile.createNewFile(), "Failed to create file: " +
splitsFile);
- splitsFilePath = splitsFile.getAbsolutePath();
+ Path splitsFile =
Files.createFile(tempDir.resolve("testSplitsFileWithNulls"));
+ assertTrue(Files.exists(splitsFile), "Failed to create file: " + file);
Review Comment:
```suggestion
assertTrue(Files.exists(splitsFile), "Failed to create file: " +
splitsFile);
```
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapred/AccumuloFileOutputFormatIT.java:
##########
@@ -175,38 +176,37 @@ public int run(String[] args) throws Exception {
public static void main(String[] args) throws Exception {
Configuration conf = new Configuration();
conf.set("mapreduce.framework.name", "local");
- conf.set("mapreduce.cluster.local.dir",
java.nio.file.Path.of(System.getProperty("user.dir"))
-
.resolve("target").resolve("mapreduce-tmp").toFile().getAbsolutePath());
+ conf.set("mapreduce.cluster.local.dir",
+ tempDir.resolve("mapreduce-tmp").toAbsolutePath().toString());
assertEquals(0, ToolRunner.run(conf, new MRTester(), args));
}
}
private void handleWriteTests(boolean content) throws Exception {
- File f = tempDir.toPath().resolve(testName()).toFile();
- assertTrue(f.createNewFile(), "Failed to create file: " + f);
- if (f.delete()) {
- log.debug("Deleted {}", f);
- }
- MRTester.main(new String[] {content ? TEST_TABLE : EMPTY_TABLE,
f.getAbsolutePath()});
-
- assertTrue(f.exists());
- File[] files = f.listFiles(file -> file.getName().startsWith("part-m-"));
- assertNotNull(files);
- if (content) {
- assertEquals(1, files.length);
- assertTrue(files[0].exists());
+ java.nio.file.Path f = tempDir.resolve(testName());
+ Files.createFile(f);
+ Files.deleteIfExists(f);
+ MRTester.main(new String[] {content ? TEST_TABLE : EMPTY_TABLE,
f.toAbsolutePath().toString()});
+
+ assertTrue(Files.exists(f));
+ try (var stream =
+ Files.list(f).filter(file ->
file.getFileName().toString().startsWith("part-m-"))) {
+ if (!content) {
+ assertTrue(stream.findAny().isEmpty());
+ return;
+ }
+ java.nio.file.Path path = stream.collect(onlyElement());
+ Files.exists(path);
Review Comment:
Is this the same logic as before? Looks like it. Was this change just to
avoid large `if` body? Also I think this is missing an assert around the
`Files.exists`
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapred/AccumuloInputFormatIT.java:
##########
@@ -55,9 +55,16 @@
import org.apache.hadoop.util.ToolRunner;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class AccumuloInputFormatIT extends AccumuloClusterHarness {
+ @SuppressFBWarnings("MS_SHOULD_BE_FINAL")
Review Comment:
Why is this needed here but not elsewhere?
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapreduce/AccumuloFileOutputFormatIT.java:
##########
@@ -192,36 +193,36 @@ public int run(String[] args) throws Exception {
public static void main(String[] args) throws Exception {
Configuration conf = new Configuration();
conf.set("mapreduce.framework.name", "local");
- conf.set("mapreduce.cluster.local.dir",
java.nio.file.Path.of(System.getProperty("user.dir"))
-
.resolve("target").resolve("mapreduce-tmp").toFile().getAbsolutePath());
+ conf.set("mapreduce.cluster.local.dir",
+ tempDir.resolve("mapreduce-tmp").toAbsolutePath().toString());
assertEquals(0, ToolRunner.run(conf, new MRTester(), args));
}
}
private void handleWriteTests(boolean content) throws Exception {
- File f = tempDir.toPath().resolve(testName()).toFile();
- assertTrue(f.createNewFile(), "Failed to create file: " + f);
- assertTrue(f.delete());
- MRTester.main(new String[] {content ? TEST_TABLE : EMPTY_TABLE,
f.getAbsolutePath()});
-
- assertTrue(f.exists());
- File[] files = f.listFiles(file -> file.getName().startsWith("part-m-"));
- assertNotNull(files);
- if (content) {
- assertEquals(1, files.length);
- assertTrue(files[0].exists());
+ java.nio.file.Path f = tempDir.resolve(testName());
+ Files.createFile(f);
+ Files.deleteIfExists(f);
+ MRTester.main(new String[] {content ? TEST_TABLE : EMPTY_TABLE,
f.toAbsolutePath().toString()});
+
+ assertTrue(Files.exists(f));
+ try (var stream = Files.list(f).filter(p ->
p.getFileName().toString().startsWith("part-m-"))) {
+ if (!content) {
+ assertTrue(stream.findAny().isEmpty());
+ return;
+ }
+ java.nio.file.Path path = stream.collect(onlyElement());
+ assertTrue(Files.exists(path));
Configuration conf = cluster.getServerContext().getHadoopConf();
DefaultConfiguration acuconf = DefaultConfiguration.getInstance();
FileSystem fs = FileSystem.getLocal(conf);
FileSKVIterator sample = RFileOperations.getInstance().newReaderBuilder()
- .forFile(UnreferencedTabletFile.of(fs, new
Path(files[0].toString())),
- FileSystem.getLocal(conf), conf, NoCryptoServiceFactory.NONE)
+ .forFile(UnreferencedTabletFile.of(fs, new Path(path.toString())),
fs, conf,
Review Comment:
was changing `FileSystem.getLocal(conf)` to `fs` intentional?
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapreduce/AccumuloFileOutputFormatIT.java:
##########
@@ -192,36 +193,36 @@ public int run(String[] args) throws Exception {
public static void main(String[] args) throws Exception {
Configuration conf = new Configuration();
conf.set("mapreduce.framework.name", "local");
- conf.set("mapreduce.cluster.local.dir",
java.nio.file.Path.of(System.getProperty("user.dir"))
-
.resolve("target").resolve("mapreduce-tmp").toFile().getAbsolutePath());
+ conf.set("mapreduce.cluster.local.dir",
+ tempDir.resolve("mapreduce-tmp").toAbsolutePath().toString());
assertEquals(0, ToolRunner.run(conf, new MRTester(), args));
}
}
private void handleWriteTests(boolean content) throws Exception {
- File f = tempDir.toPath().resolve(testName()).toFile();
- assertTrue(f.createNewFile(), "Failed to create file: " + f);
- assertTrue(f.delete());
- MRTester.main(new String[] {content ? TEST_TABLE : EMPTY_TABLE,
f.getAbsolutePath()});
-
- assertTrue(f.exists());
- File[] files = f.listFiles(file -> file.getName().startsWith("part-m-"));
- assertNotNull(files);
- if (content) {
- assertEquals(1, files.length);
- assertTrue(files[0].exists());
+ java.nio.file.Path f = tempDir.resolve(testName());
+ Files.createFile(f);
+ Files.deleteIfExists(f);
Review Comment:
Same delete after create question
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapred/MultiTableInputFormatIT.java:
##########
@@ -45,9 +45,16 @@
import org.apache.hadoop.util.Tool;
import org.apache.hadoop.util.ToolRunner;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class MultiTableInputFormatIT extends AccumuloClusterHarness {
+ @SuppressFBWarnings("MS_SHOULD_BE_FINAL")
Review Comment:
Same question
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapreduce/AccumuloFileOutputFormatIT.java:
##########
@@ -233,11 +234,11 @@ private void handleWriteTests(boolean content) throws
Exception {
@Test
public void writeBadVisibility() throws Exception {
- File f = tempDir.toPath().resolve(testName()).toFile();
- assertTrue(f.createNewFile(), "Failed to create file: " + f);
- assertTrue(f.delete());
- MRTester.main(new String[] {BAD_TABLE, f.getAbsolutePath()});
- assertTrue(f.exists());
+ java.nio.file.Path f = tempDir.resolve(testName());
+ Files.createFile(f);
+ Files.deleteIfExists(f);
Review Comment:
same delete after create question
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapreduce/AccumuloInputFormatIT.java:
##########
@@ -69,17 +69,24 @@
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.Multimap;
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
/**
* Tests the new MR API in the hadoop-mapreduce package.
*
* @since 2.0
*/
public class AccumuloInputFormatIT extends SharedMiniClusterBase {
+ @SuppressFBWarnings("MS_SHOULD_BE_FINAL")
Review Comment:
Same suppression question
##########
hadoop-mapreduce/src/test/java/org/apache/accumulo/hadoop/its/mapreduce/MultiTableInputFormatIT.java:
##########
@@ -42,9 +42,16 @@
import org.apache.hadoop.util.Tool;
import org.apache.hadoop.util.ToolRunner;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
public class MultiTableInputFormatIT extends AccumuloClusterHarness {
+ @SuppressFBWarnings("MS_SHOULD_BE_FINAL")
Review Comment:
same suppression question
##########
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneAccumuloCluster.java:
##########
@@ -246,15 +246,14 @@ public AccumuloConfiguration getSiteConfiguration() {
justification = "code runs in same security context as user who provided
input file name")
@Override
public String getAccumuloPropertiesPath() {
- return
java.nio.file.Path.of(serverAccumuloConfDir).resolve("accumulo.properties").toFile()
- .toString();
+ return
java.nio.file.Path.of(serverAccumuloConfDir).resolve("accumulo.properties").toString();
}
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN",
justification = "code runs in same security context as user who provided
input file name")
@Override
public String getClientPropsPath() {
return
java.nio.file.Path.of(clientAccumuloConfDir).resolve("accumulo-client.properties")
- .toFile().toString();
+ .toString();
}
Review Comment:
Could these methods instead return Path?
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -490,8 +496,9 @@ private void writeConfig(File file,
Iterable<Map.Entry<String,String>> settings)
fileWriter.close();
}
- private void writeConfigProperties(File file, Map<String,String> settings)
throws IOException {
- BufferedWriter fileWriter = Files.newBufferedWriter(file.toPath());
+ private void writeConfigProperties(java.nio.file.Path file,
Map<String,String> settings)
Review Comment:
I don't think the change from File -> Path here made any difference for
`writeConfigProperties` and `writeConfig`. Conversions are done from File ->
Path when passing an arg sometimes and conversions are done from Path -> File
after the method call.
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloConfigImpl.java:
##########
@@ -756,7 +758,7 @@ public boolean useExistingInstance() {
* @since 1.6.2
*/
public File getHadoopConfDir() {
- return this.hadoopConfDir;
+ return dir.toFile();
Review Comment:
Returning wrong value
##########
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterClasspathTest.java:
##########
@@ -65,14 +66,15 @@ public class MiniAccumuloClusterClasspathTest extends
WithTestNames {
@BeforeAll
public static void setupMiniCluster() throws Exception {
- File baseDir =
-
Path.of(System.getProperty("user.dir")).resolve("target").resolve("mini-tests").toFile();
- assertTrue(baseDir.mkdirs() || baseDir.isDirectory());
- testDir =
baseDir.toPath().resolve(MiniAccumuloClusterTest.class.getName()).toFile();
+ Path baseDir =
Path.of(System.getProperty("user.dir")).resolve("target").resolve("mini-tests");
+ if (!Files.isDirectory(baseDir)) {
+ Files.createDirectories(baseDir);
+ }
+ testDir =
baseDir.resolve(MiniAccumuloClusterTest.class.getName()).toFile();
Review Comment:
probably still want the assertion or similar
##########
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterStartStopTest.java:
##########
@@ -40,17 +40,18 @@ public class MiniAccumuloClusterStartStopTest extends
WithTestNames {
private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterStartStopTest.class);
- private File baseDir =
Path.of(System.getProperty("user.dir")).resolve("target")
- .resolve("mini-tests").resolve(this.getClass().getName()).toFile();
+ @TempDir
+ private static Path tempDir;
+
private MiniAccumuloCluster accumulo;
@BeforeEach
public void setupTestCluster() throws IOException {
- assertTrue(baseDir.mkdirs() || baseDir.isDirectory());
- File testDir = baseDir.toPath().resolve(testName()).toFile();
- FileUtils.deleteQuietly(testDir);
- assertTrue(testDir.mkdir());
- accumulo = new MiniAccumuloCluster(testDir, "superSecret");
+ assertTrue(Files.isDirectory(tempDir));
Review Comment:
Here as well
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -260,12 +261,12 @@ public MiniAccumuloClusterImpl(MiniAccumuloConfigImpl
config) throws IOException
clientProps.put(ClientProperty.AUTH_TOKEN.getKey(),
config.getRootPassword());
}
- File clientPropsFile = config.getClientPropsFile();
+ java.nio.file.Path clientPropsFile = config.getClientPropsFile().toPath();
Review Comment:
This converts from file to path. Is it possible to just keep it as file here?
##########
minicluster/src/test/java/org/apache/accumulo/miniclusterImpl/CleanShutdownMacTest.java:
##########
@@ -41,13 +41,15 @@
public class CleanShutdownMacTest extends WithTestNames {
@TempDir
- private static File tmpDir;
+ private static Path tmpDir;
@Test
public void testExecutorServiceShutdown() throws Exception {
- File tmp = tmpDir.toPath().resolve(testName()).toFile();
- assertTrue(tmp.isDirectory() || tmp.mkdir(), "Failed to make a new
sub-directory");
Review Comment:
might still be good to have a similar assertion
##########
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java:
##########
@@ -32,36 +33,36 @@
import org.apache.accumulo.core.clientImpl.Namespace;
import org.apache.accumulo.core.clientImpl.NamespaceMapping;
import org.apache.accumulo.core.fate.zookeeper.ZooUtil;
-import org.apache.commons.io.FileUtils;
import org.apache.curator.framework.CuratorFramework;
import org.apache.curator.framework.CuratorFrameworkFactory;
import org.apache.curator.retry.RetryOneTime;
import org.apache.curator.test.TestingServer;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN", justification = "paths not
set by user input")
public class MiniAccumuloClusterExistingZooKeepersTest extends WithTestNames {
- private static final File BASE_DIR =
-
Path.of(System.getProperty("user.dir")).resolve("target").resolve("mini-tests")
-
.resolve(MiniAccumuloClusterExistingZooKeepersTest.class.getName()).toFile();
+
+ @TempDir
+ private static Path tempDir;
private static final String SECRET = "superSecret";
private MiniAccumuloConfig config;
@BeforeEach
- public void setupTestCluster() {
- assertTrue(BASE_DIR.mkdirs() || BASE_DIR.isDirectory());
- File testDir = BASE_DIR.toPath().resolve(testName()).toFile();
- FileUtils.deleteQuietly(testDir);
- assertTrue(testDir.mkdir());
+ public void setupTestCluster() throws IOException {
+ assertTrue(Files.isDirectory(tempDir));
Review Comment:
I don't think the tempDir has been created yet so this won't pass. I'm not
entirely sure how @ TempDir works though
##########
minicluster/src/main/java/org/apache/accumulo/miniclusterImpl/MiniAccumuloClusterImpl.java:
##########
@@ -260,12 +261,12 @@ public MiniAccumuloClusterImpl(MiniAccumuloConfigImpl
config) throws IOException
clientProps.put(ClientProperty.AUTH_TOKEN.getKey(),
config.getRootPassword());
}
- File clientPropsFile = config.getClientPropsFile();
+ java.nio.file.Path clientPropsFile = config.getClientPropsFile().toPath();
writeConfigProperties(clientPropsFile, clientProps);
- File siteFile = confDir.resolve("accumulo.properties").toFile();
+ java.nio.file.Path siteFile = confDir.resolve("accumulo.properties");
writeConfigProperties(siteFile, config.getSiteConfig());
- this.siteConfig = SiteConfiguration.fromFile(siteFile).build();
+ this.siteConfig = SiteConfiguration.fromFile(siteFile.toFile()).build();
Review Comment:
siteFile is still converted from path to file. Is it possible to keep as
only file or only path?
##########
server/base/src/test/java/org/apache/accumulo/server/conf/CheckCompactionConfigTest.java:
##########
@@ -168,13 +168,15 @@ public void testBadPropsFilePath() {
}
private String writeToFileAndReturnPath(String inputString) throws
IOException {
- File file = tempDir.toPath().resolve(testName() + ".properties").toFile();
- assertTrue(file.isFile() || file.createNewFile());
- try (BufferedWriter bufferedWriter =
Files.newBufferedWriter(file.toPath())) {
+ Path file = tempDir.resolve(testName() + ".properties");
+ if (!Files.isRegularFile(file)) {
+ Files.createFile(file);
+ }
Review Comment:
would be good to keep a similar assertion
##########
test/src/main/java/org/apache/accumulo/harness/TestingKdc.java:
##########
@@ -52,48 +54,54 @@ public class TestingKdc {
public final String ORG_NAME = "EXAMPLE", ORG_DOMAIN = "COM";
private String hostname;
- private File keytabDir;
+ private Path keytabDir;
private boolean started = false;
public TestingKdc() throws Exception {
this(computeKdcDir(), computeKeytabDir(), MAX_TICKET_LIFETIME_MILLIS);
}
- public static File computeKdcDir() {
+ public static Path computeKdcDir() throws IOException {
Path userDir = Path.of(System.getProperty("user.dir"));
- File targetDir = userDir.resolve("target").toFile();
- if (!targetDir.exists()) {
- assertTrue(targetDir.mkdirs());
+ Path targetDir = userDir.resolve("target");
+ if (!Files.exists(targetDir)) {
+ Files.createDirectories(targetDir);
}
- assertTrue(targetDir.isDirectory(), "Could not find Maven target
directory: " + targetDir);
+ assertTrue(Files.isDirectory(targetDir), "Could not find Maven target
directory: " + targetDir);
// Create the directories: target/kerberos/minikdc
- File kdcDir =
targetDir.toPath().resolve("kerberos").resolve("minikdc").toFile();
+ Path kdcDir = targetDir.resolve("kerberos").resolve("minikdc");
- assertTrue(kdcDir.mkdirs() || kdcDir.isDirectory());
+ if (!Files.isDirectory(kdcDir)) {
+ Files.createDirectories(kdcDir);
+ }
return kdcDir;
}
- public static File computeKeytabDir() {
+ public static Path computeKeytabDir() throws IOException {
Path userDir = Path.of(System.getProperty("user.dir"));
- File targetDir = userDir.resolve("target").toFile();
- assertTrue(targetDir.exists() && targetDir.isDirectory(),
- "Could not find Maven target directory: " + targetDir);
+ Path targetDir = userDir.resolve("target");
+ if (!Files.exists(targetDir)) {
+ Files.createDirectories(targetDir);
+ }
+ assertTrue(Files.isDirectory(targetDir), "Could not find Maven target
directory: " + targetDir);
// Create the directories: target/kerberos/keytabs
- File keytabDir =
targetDir.toPath().resolve("kerberos").resolve("keytabs").toFile();
+ Path keytabDir = targetDir.resolve("kerberos").resolve("keytabs");
- assertTrue(keytabDir.mkdirs() || keytabDir.isDirectory());
+ if (!Files.isDirectory(keytabDir)) {
+ Files.createDirectories(keytabDir);
+ }
Review Comment:
could add similar assertion
##########
server/base/src/test/java/org/apache/accumulo/server/security/SystemCredentialsTest.java:
##########
@@ -51,21 +51,22 @@ public static void setUp() throws IOException {
Path targetDir = Path.of("target");
Path instanceDir = targetDir.resolve("instanceTest");
Path instanceIdDir = instanceDir.resolve(Constants.INSTANCE_ID_DIR);
- File testInstanceId = instanceIdDir
-
.resolve(UUID.fromString("00000000-0000-0000-0000-000000000000").toString()).toFile();
- if (!testInstanceId.exists()) {
- assertTrue(
- testInstanceId.getParentFile().mkdirs() ||
testInstanceId.getParentFile().isDirectory());
- assertTrue(testInstanceId.createNewFile());
- }
+ Path testInstanceId =
+
instanceIdDir.resolve(UUID.fromString("00000000-0000-0000-0000-000000000000").toString());
+ ensureFileExists(testInstanceId);
Path versionDir = instanceDir.resolve(Constants.VERSION_DIR);
+ Path testInstanceVersion = versionDir.resolve(AccumuloDataVersion.get() +
"");
+ ensureFileExists(testInstanceVersion);
+ }
- File testInstanceVersion = versionDir.resolve(AccumuloDataVersion.get() +
"").toFile();
- if (!testInstanceVersion.exists()) {
- assertTrue(testInstanceVersion.getParentFile().mkdirs()
- || testInstanceVersion.getParentFile().isDirectory());
- assertTrue(testInstanceVersion.createNewFile());
+ private static void ensureFileExists(Path testInstanceVersion) throws
IOException {
+ if (!Files.exists(testInstanceVersion)) {
+ Path parentDir = testInstanceVersion.getParent();
+ if (parentDir != null && !Files.isDirectory(parentDir)) {
+ Files.createDirectories(parentDir);
+ }
+ Files.createFile(testInstanceVersion);
Review Comment:
parameter name could be more generic
also, it doesn't seem like this is equivalent. Before, the parent directory
was created if it doesn't exist, now that isn't the case.
##########
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterTest.java:
##########
@@ -67,20 +67,17 @@ public class MiniAccumuloClusterTest extends WithTestNames {
public static final String ROOT_PASSWORD = "superSecret";
public static final String ROOT_USER = "root";
+ @TempDir
+ private static Path tempDir;
+
public static File testDir;
Review Comment:
Is this still used?
##########
test/src/main/java/org/apache/accumulo/harness/TestingKdc.java:
##########
@@ -52,48 +54,54 @@ public class TestingKdc {
public final String ORG_NAME = "EXAMPLE", ORG_DOMAIN = "COM";
private String hostname;
- private File keytabDir;
+ private Path keytabDir;
private boolean started = false;
public TestingKdc() throws Exception {
this(computeKdcDir(), computeKeytabDir(), MAX_TICKET_LIFETIME_MILLIS);
}
- public static File computeKdcDir() {
+ public static Path computeKdcDir() throws IOException {
Path userDir = Path.of(System.getProperty("user.dir"));
- File targetDir = userDir.resolve("target").toFile();
- if (!targetDir.exists()) {
- assertTrue(targetDir.mkdirs());
+ Path targetDir = userDir.resolve("target");
+ if (!Files.exists(targetDir)) {
+ Files.createDirectories(targetDir);
}
- assertTrue(targetDir.isDirectory(), "Could not find Maven target
directory: " + targetDir);
+ assertTrue(Files.isDirectory(targetDir), "Could not find Maven target
directory: " + targetDir);
// Create the directories: target/kerberos/minikdc
- File kdcDir =
targetDir.toPath().resolve("kerberos").resolve("minikdc").toFile();
+ Path kdcDir = targetDir.resolve("kerberos").resolve("minikdc");
- assertTrue(kdcDir.mkdirs() || kdcDir.isDirectory());
+ if (!Files.isDirectory(kdcDir)) {
+ Files.createDirectories(kdcDir);
+ }
Review Comment:
could add similar assertion
##########
test/src/main/java/org/apache/accumulo/harness/TestingKdc.java:
##########
@@ -52,48 +54,54 @@ public class TestingKdc {
public final String ORG_NAME = "EXAMPLE", ORG_DOMAIN = "COM";
private String hostname;
- private File keytabDir;
+ private Path keytabDir;
private boolean started = false;
public TestingKdc() throws Exception {
this(computeKdcDir(), computeKeytabDir(), MAX_TICKET_LIFETIME_MILLIS);
}
- public static File computeKdcDir() {
+ public static Path computeKdcDir() throws IOException {
Path userDir = Path.of(System.getProperty("user.dir"));
- File targetDir = userDir.resolve("target").toFile();
- if (!targetDir.exists()) {
- assertTrue(targetDir.mkdirs());
+ Path targetDir = userDir.resolve("target");
+ if (!Files.exists(targetDir)) {
+ Files.createDirectories(targetDir);
}
- assertTrue(targetDir.isDirectory(), "Could not find Maven target
directory: " + targetDir);
+ assertTrue(Files.isDirectory(targetDir), "Could not find Maven target
directory: " + targetDir);
// Create the directories: target/kerberos/minikdc
- File kdcDir =
targetDir.toPath().resolve("kerberos").resolve("minikdc").toFile();
+ Path kdcDir = targetDir.resolve("kerberos").resolve("minikdc");
- assertTrue(kdcDir.mkdirs() || kdcDir.isDirectory());
+ if (!Files.isDirectory(kdcDir)) {
+ Files.createDirectories(kdcDir);
+ }
return kdcDir;
}
- public static File computeKeytabDir() {
+ public static Path computeKeytabDir() throws IOException {
Path userDir = Path.of(System.getProperty("user.dir"));
- File targetDir = userDir.resolve("target").toFile();
- assertTrue(targetDir.exists() && targetDir.isDirectory(),
- "Could not find Maven target directory: " + targetDir);
+ Path targetDir = userDir.resolve("target");
+ if (!Files.exists(targetDir)) {
+ Files.createDirectories(targetDir);
+ }
+ assertTrue(Files.isDirectory(targetDir), "Could not find Maven target
directory: " + targetDir);
Review Comment:
This kind of thing is done a lot. I wonder if you can move this to a method
in some file utility class
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
