mlbiscoc commented on code in PR #3263:
URL: https://github.com/apache/solr/pull/3263#discussion_r1999375059
##########
solr/core/src/test/org/apache/solr/search/TestRecovery.java:
##########
@@ -1779,11 +1778,11 @@ public void testRecoveryMultipleLogs() throws Exception
{
assertU(adoc("id", "CCCCCC"));
h.close();
- String[] files = ulog.getLogList(logDir.toPath());
+ String[] files = ulog.getLogList(logDir);
Arrays.sort(files);
String fname = files[files.length - 1];
byte[] content;
- try (RandomAccessFile raf = new RandomAccessFile(new File(logDir,
fname), "rw")) {
+ try (RandomAccessFile raf = new
RandomAccessFile(logDir.resolve(fname).toFile(), "rw")) {
Review Comment:
Still trying to figure out the the NIO Path equivalent is here. Going to
leave this comment to come back
##########
solr/core/src/test/org/apache/solr/cli/StreamToolTest.java:
##########
@@ -185,11 +183,11 @@ public void testReadStream() throws Exception {
@Test
@SuppressWarnings({"unchecked", "rawtypes"})
public void testLocalCatStream() throws Exception {
- File localFile = File.createTempFile("topLevel1", ".txt");
- populateFileWithData(localFile.toPath());
+ Path localFile = Files.createTempFile("topLevel1", ".txt");
+ populateFileWithData(localFile);
StreamTool.LocalCatStream catStream =
- new StreamTool.LocalCatStream(localFile.getAbsolutePath(), -1);
+ new StreamTool.LocalCatStream(localFile.toAbsolutePath().toString(),
-1);
Review Comment:
We can but would required some refactor of
[CatStream](https://github.com/apache/solr/blob/af97ef7037aec0c1fb028e173b6d8931cd12588d/solr/core/src/java/org/apache/solr/handler/CatStream.java#L52)
which does a bunch of string parsing and changes the path string and adds
comma delimiters. Not sure how Path would change the underlying path, if at all.
##########
solr/core/src/test/org/apache/solr/SolrInfoBeanTest.java:
##########
@@ -88,30 +90,37 @@ public void testCallMBeanInfo() throws Exception {
}
private static List<Class<?>> getClassesForPackage(String pckgname) throws
Exception {
- ArrayList<File> directories = new ArrayList<>();
+ ArrayList<Path> directories = new ArrayList<>();
ClassLoader cld = h.getCore().getResourceLoader().getClassLoader();
String path = pckgname.replace('.', '/');
Enumeration<URL> resources = cld.getResources(path);
while (resources.hasMoreElements()) {
final URI uri = resources.nextElement().toURI();
if (!"file".equalsIgnoreCase(uri.getScheme())) continue;
- final File f = new File(uri);
+ final Path f = Path.of(uri);
directories.add(f);
}
ArrayList<Class<?>> classes = new ArrayList<>();
- for (File directory : directories) {
- if (directory.exists()) {
- String[] files = directory.list();
- for (String file : files) {
- if (file.endsWith(".class")) {
- String clazzName = file.substring(0, file.length() - 6);
- // exclude Test classes that happen to be in these packages.
- // class.ForName'ing some of them can cause trouble.
- if (!clazzName.endsWith("Test") && !clazzName.startsWith("Test")) {
- classes.add(Class.forName(pckgname + '.' + clazzName));
- }
- }
+ for (Path directory : directories) {
+ if (Files.exists(directory)) {
+ try (Stream<Path> files = Files.list(directory)) {
Review Comment:
The Path equivalent for `directory.list();` in path is `Files.list` but this
requires a try-with-resources since it returns a stream and the try with close
the stream at the end. Other option is removing the try with resources but we
need to call a `.close()` to close it manually.
##########
solr/core/src/test/org/apache/solr/cli/TestSolrCLIRunExample.java:
##########
@@ -558,32 +555,34 @@ public void testInteractiveSolrCloudExample() throws
Exception {
@Test
public void testFailExecuteScript() throws Exception {
- File solrHomeDir = new File(ExternalPaths.SERVER_HOME);
- if (!solrHomeDir.isDirectory())
- fail(solrHomeDir.getAbsolutePath() + " not found and is required to run
this test!");
+ Path solrHomeDir = Path.of(ExternalPaths.SERVER_HOME);
+ if (!Files.isDirectory(solrHomeDir))
+ fail(solrHomeDir.toAbsolutePath() + " not found and is required to run
this test!");
- Path tmpDir = createTempDir();
- File solrExampleDir = tmpDir.toFile();
- File solrServerDir = solrHomeDir.getParentFile();
+ Path solrExampleDir = createTempDir();
+ Path solrServerDir = solrHomeDir.getParent();
// need a port to start the example server on
int bindPort = -1;
try (ServerSocket socket = new ServerSocket(0)) {
bindPort = socket.getLocalPort();
}
- File toExecute = new File(tmpDir.toString(), "failExecuteScript");
- assertTrue(
- "Should have been able to create file '" + toExecute.getAbsolutePath()
+ "' ",
- toExecute.createNewFile());
+ Path toExecute = Path.of(solrExampleDir.toString(), "failExecuteScript");
+ try {
+ Files.createFile(toExecute);
+ } catch (IOException e) {
+ throw new IOException(
+ "Should have been able to create file '" +
toExecute.toAbsolutePath() + "' ");
+ }
String[] toolArgs =
new String[] {
"-e", "techproducts",
- "--server-dir", solrServerDir.getAbsolutePath(),
- "--example-dir", solrExampleDir.getAbsolutePath(),
+ "--server-dir", solrServerDir.toAbsolutePath().toString(),
Review Comment:
Don't think there is an easy unless `Path` inherits String class so they can
share the array.
##########
solr/modules/sql/src/test/org/apache/solr/handler/sql/TestSQLHandlerNonCloud.java:
##########
@@ -32,17 +32,17 @@
public class TestSQLHandlerNonCloud extends SolrJettyTestBase {
- private static File createSolrHome() throws Exception {
- File workDir = createTempDir().toFile();
+ private static Path createSolrHome() throws Exception {
+ Path workDir = createTempDir();
setupJettyTestHome(workDir, DEFAULT_TEST_COLLECTION_NAME);
return workDir;
}
@BeforeClass
public static void beforeClass() throws Exception {
- File solrHome = createSolrHome();
- solrHome.deleteOnExit();
- createAndStartJetty(solrHome.getAbsolutePath());
+ Path solrHome = createSolrHome();
+ solrHome.toFile().deleteOnExit();
Review Comment:
Can't find a Path equivalent for this. Looking at posts on stack, you either
do the way above or write a shutdown hook too cleanup the file on exit
##########
solr/core/src/test/org/apache/solr/cli/SolrProcessManagerTest.java:
##########
@@ -84,9 +83,11 @@ private static int findAvailablePort() throws IOException {
private static Pair<Integer, Process> createProcess(int port, boolean https)
throws IOException {
// Get the path to the java executable from the current JVM
String classPath =
-
Arrays.stream(System.getProperty("java.class.path").split(File.pathSeparator))
+ Arrays.stream(
+
System.getProperty("java.class.path").split(System.getProperty("path.separator")))
Review Comment:
Couldn't find an easy way to remove this.
`FileSystems.getDefault().getSeparator()` gives you separator for a path such
as `/` but the File.pathSeparator is a character used to separate multiple file
paths in a string such as `:` and on windows I think it is `;`? I didn't like
it either when I put this but wasn't sure what other option to put.
##########
solr/core/src/test/org/apache/solr/cli/SolrCLIZkToolsTest.java:
##########
@@ -417,7 +434,7 @@ public void testCp() throws Exception {
res = cpTool.runTool(SolrCLI.processCommandLineArgs(cpTool, args));
assertEquals("Copy should have succeeded.", 0, res);
- Path locEmpty = Paths.get(tmp2.toAbsolutePath().toString(), "stopwords",
"emptyfile");
+ Path locEmpty = Path.of(tmp2.toAbsolutePath().toString(), "stopwords",
"emptyfile");
Review Comment:
Yeah I try to move to resolve as much as possible, but I might have some
mistakes like the one you found here. Tried my best to clean up some of it.
##########
solr/core/src/test/org/apache/solr/cli/SolrCLIZkToolsTest.java:
##########
@@ -265,7 +275,10 @@ public void testCp() throws Exception {
new String[] {
"--zk-host",
zkAddr,
- "file:" + srcPathCheck.normalize().toAbsolutePath() + File.separator
+ "solrconfig.xml",
+ "file:"
+ + srcPathCheck.normalize().toAbsolutePath()
+ + FileSystems.getDefault().getSeparator()
Review Comment:
Tried to remove as much as `FileSystems.getDefault().getSeparator()` around
the PR as I could.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
