DomGarguilo commented on code in PR #5547:
URL: https://github.com/apache/accumulo/pull/5547#discussion_r2121479004
##########
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 don't think so. `deleteOnExit()` adds a shutdown hook that deletes the
file when the JVM shuts down. JUnits `@TempDir` creates a new dir for each test
and deletes it after the test runs. So it functions very similar from the
perspective of the test, but we don't need to worry about any of the manual
cleanup.
##########
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:
Yea, since we already had it in the `fs` variable, figured I would just
reuse that instead.
##########
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:
I was able to just remove this actually. I used the `tempDir` which
simplified things too.
##########
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:
`createDirectories()` will throw an exception if it fails to properly set up
the path as a directory.
##########
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:
Yea, it will throw an exception if the file cannot be created for any reason
which will make the test fail.
##########
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:
Maybe in the future but right now `writeConfigProperties()` accepts a Path
as a param and `SiteConfiguration.fromFile()` accepts a `File` as a param
##########
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:
I looked into `createDirectories()` and it will throw an exception if it
fails to properly create the directory. So if no exception is thrown, we can
assume the directory was properly set up.
##########
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:
I think it makes more sense to have `dir` as a `Path` internally since we
construct so many sub-dirs within it we can avoid a bunch of calls to
`dir.toPath()`. It would be nice to make the return type of the various sub-dir
getters a `Path` object but I didn't want to make that change in this PR.
##########
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:
`writeConfigProperties()` accepts Path as a param now so we need to convert
it until getClientPropsFile() returns a `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:
Thats fair. I made this change as a preemptive step towards converting more
things to `Path`. I didn't want to change the return type of certain things in
this PR but eventually, if more things use `Path` then the changes will already
have been made to make that transition easier.
##########
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:
These files were actually not being used at all in the test. I was able to
remove the whole setup block.
--
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]
