dsmiley commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1885858097
##########
solr/core/src/java/org/apache/solr/spelling/suggest/fst/AnalyzingInfixLookupFactory.java:
##########
@@ -108,7 +109,7 @@ public Lookup create(NamedList<?> params, SolrCore core) {
try {
return new AnalyzingInfixSuggester(
- FSDirectory.open(new File(indexPath).toPath()),
Review Comment:
Wow that line of code was sad.
##########
solr/core/src/java/org/apache/solr/util/FileUtils.java:
##########
@@ -37,14 +36,14 @@ public class FileUtils {
* If path is absolute, then a File for that path is returned; if it's not
absolute, then a File
* is returned using "path" as a child of "base")
*/
- public static File resolvePath(File base, String path) {
- File r = new File(path);
- return r.isAbsolute() ? r : new File(base, path);
+ public static Path resolvePath(Path base, String path) {
+ Path r = Path.of(path);
+ return r.isAbsolute() ? r : Path.of(base.toString(), path);
}
- public static void copyFile(File src, File destination) throws IOException {
- try (FileChannel in = new FileInputStream(src).getChannel();
- FileChannel out = new FileOutputStream(destination).getChannel()) {
+ public static void copyFile(Path src, Path destination) throws IOException {
Review Comment:
The only reason this method exists, is because the inputs were File and not
Path. See Files.copy(path, path). So at this point, you can just write that
one-liner then inline this method (IntelliJ will do for you). IntelliJ kicks
but at refactoring; perhaps Eclipse does fine too; I dunno.
##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -1518,12 +1521,14 @@ private void copyTmpConfFiles2Conf(Path tmpconfDir) {
* backup of the old directory is maintained. If the directory move fails,
it will try to revert
* back the original tlog directory.
*/
- private boolean copyTmpTlogFiles2Tlog(File tmpTlogDir) {
+ private boolean copyTmpTlogFiles2Tlog(Path tmpTlogDir) {
Path tlogDir =
FileSystems.getDefault().getPath(solrCore.getUpdateHandler().getUpdateLog().getTlogDir());
Path backupTlogDir =
FileSystems.getDefault()
- .getPath(tlogDir.getParent().toAbsolutePath().toString(),
tmpTlogDir.getName());
+ .getPath(
+ tlogDir.getParent().toAbsolutePath().toString(),
+ tmpTlogDir.getFileName().toString());
Review Comment:
This looks more complicated than it needs to be. It's highly unusual for
anyone to need to refer to FileSystems. Look at the implementation of
`Path.of(...)` which proves we can just use Path.of here.
But hey, copyTmpTlogFiles2Tlog isn't used at all so let's just delete it!
##########
solr/modules/analysis-extras/src/test/org/apache/solr/schema/TestICUCollationFieldOptions.java:
##########
@@ -26,7 +26,7 @@ public class TestICUCollationFieldOptions extends
SolrTestCaseJ4 {
@BeforeClass
public static void beforeClass() throws Exception {
File testHome = createTempDir().toFile();
- FileUtils.copyDirectory(getFile("analysis-extras/solr"), testHome);
+ FileUtils.copyDirectory(getFile("analysis-extras/solr").toFile(),
testHome);
Review Comment:
(same as my last comment)
##########
solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java:
##########
@@ -172,16 +172,23 @@ public void configure(SolrResourceLoader loader,
NamedList<String> initArgs)
throw new IllegalArgumentException(
"Required configuration parameter '" + STORAGE_DIR_INIT_ARG + "'
not provided!");
- File dir = new File(storageDirArg);
- if (!dir.isDirectory()) dir.mkdirs();
+ Path dir = Path.of(storageDirArg);
+ if (!Files.isDirectory(dir)) {
+ try {
+ Files.createDirectories(dir);
+ } catch (IOException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Unable to create storage
directory " + dir, e);
+ }
+ }
- storageDir = dir.getAbsolutePath();
+ storageDir = dir.toAbsolutePath().toString();
log.info("File-based storage initialized to use dir: {}", storageDir);
}
@Override
public boolean exists(String storedResourceId) throws IOException {
- return (new File(storageDir, storedResourceId)).exists();
+ return (Files.exists(Path.of(storageDir, storedResourceId)));
Review Comment:
there's a needless parenthesis here
##########
solr/core/src/java/org/apache/solr/core/StandardDirectoryFactory.java:
##########
@@ -78,7 +77,7 @@ protected LockFactory createLockFactory(String rawLockType)
throws IOException {
@Override
public String normalize(String path) throws IOException {
- return super.normalize(new File(path).getCanonicalPath());
+ return super.normalize(Path.of(path).toAbsolutePath().toString());
Review Comment:
File.getCanonicalPath is not identical to Path.toAbsolutePath. It requires
adding normalize().
##########
solr/core/src/java/org/apache/solr/util/FileUtils.java:
##########
@@ -55,16 +54,16 @@ public static void copyFile(File src, File destination)
throws IOException {
* @param fullFile the File to be synced to disk
* @throws IOException if the file could not be synced
*/
- public static void sync(File fullFile) throws IOException {
- if (fullFile == null || !fullFile.exists())
+ public static void sync(Path fullFile) throws IOException {
Review Comment:
Use of RandomAccessFile here is super suspicious to me. So I see the
javadocs of this method show this is copied code. I went and took a look and I
now see that Lucene has a utility that is exposed:
org.apache.lucene.util.IOUtils#fsync. So we don't need this method anymore.
##########
solr/core/src/test/org/apache/solr/util/FileUtilsTest.java:
##########
@@ -25,11 +25,11 @@ public class FileUtilsTest extends SolrTestCase {
@Test
public void testResolve() {
Review Comment:
FileUtils.resolvePath exists only because of the File API. We don't need
it; don't need to test it either.
##########
solr/core/src/java/org/apache/solr/update/UpdateLog.java:
##########
@@ -731,11 +730,11 @@ private boolean updateFromOldTlogs(UpdateCommand cmd) {
return (cmd.getFlags() & UpdateCommand.REPLAY) != 0 && state ==
State.REPLAYING;
}
- public String[] getLogList(File directory) {
+ public String[] getLogList(Path directory) {
final String prefix = TLOG_NAME + '.';
- String[] names = directory.list((dir, name) -> name.startsWith(prefix));
+ String[] names = directory.toFile().list((dir, name) ->
name.startsWith(prefix));
Review Comment:
Please don't call toFile
##########
solr/core/src/test/org/apache/solr/schema/ExternalFileFieldSortTest.java:
##########
@@ -26,7 +26,7 @@
public class ExternalFileFieldSortTest extends SolrTestCaseJ4 {
static void updateExternalFile() throws IOException {
- final String testHome =
SolrTestCaseJ4.getFile("solr/collection1").getParent();
+ final String testHome =
SolrTestCaseJ4.getFile("solr/collection1").getParent().toString();
String filename = "external_eff";
Files.copy(Path.of(testHome, filename), Path.of(h.getCore().getDataDir(),
filename));
Review Comment:
testHome needn't be of type String; let it be Path. In the line below, call
testHome.resolve(filename).
Part of the point of Path is that it's easier (than File) to use the typed
variants (Path) instead of relatively untyped String.
##########
solr/modules/analysis-extras/src/test/org/apache/solr/analysis/TestFoldingMultitermExtrasQuery.java:
##########
@@ -33,7 +33,7 @@ public String getCoreName() {
@BeforeClass
public static void beforeTests() throws Exception {
File testHome = createTempDir().toFile();
- FileUtils.copyDirectory(getFile("analysis-extras/solr"), testHome);
+ FileUtils.copyDirectory(getFile("analysis-extras/solr").toFile(),
testHome);
Review Comment:
Skip File conversion and use PathUtils.copyDirectory
##########
solr/solrj/src/test/org/apache/solr/client/solrj/request/TestConfigSetAdminRequest.java:
##########
@@ -41,7 +41,7 @@ public void testUpload() throws Exception {
upload.setConfigSetName("name");
verifyException(upload, "There must be a ContentStream");
- upload.setUploadFile(tmpFile, "application/zip");
+ upload.setUploadFile(tmpFile.toPath(), "application/zip");
Review Comment:
simply declare tmpFile as Path without conversion and maybe no further
changes
##########
solr/modules/clustering/src/test/org/apache/solr/handler/clustering/ClusteringComponentTest.java:
##########
@@ -56,7 +56,7 @@ public class ClusteringComponentTest extends SolrTestCaseJ4 {
@BeforeClass
public static void beforeClass() throws Exception {
File testHome = createTempDir().toFile();
- FileUtils.copyDirectory(getFile("clustering/solr"), testHome);
+ FileUtils.copyDirectory(getFile("clustering/solr").toFile(), testHome);
Review Comment:
same
##########
solr/modules/analysis-extras/src/test/org/apache/solr/update/processor/TestOpenNLPExtractNamedEntitiesUpdateProcessorFactory.java:
##########
@@ -29,7 +29,7 @@ public class
TestOpenNLPExtractNamedEntitiesUpdateProcessorFactory extends Updat
@BeforeClass
public static void beforeClass() throws Exception {
File testHome = createTempDir().toFile();
- FileUtils.copyDirectory(getFile("analysis-extras/solr"), testHome);
+ FileUtils.copyDirectory(getFile("analysis-extras/solr").toFile(),
testHome);
Review Comment:
(same)
--
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]
