You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/01/19 21:25:58 UTC
[GitHub] [solr] madrob opened a new pull request #544: SOLR-15919 Another pass as moving File to Path
madrob opened a new pull request #544:
URL: https://github.com/apache/solr/pull/544
https://issues.apache.org/jira/browse/SOLR-15919
I'm happy to create a new JIRA if people prefer or we can lump it in with the previous one.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #544: SOLR-15919 Another pass as moving File to Path
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #544:
URL: https://github.com/apache/solr/pull/544#discussion_r788256229
##########
File path: solr/core/src/java/org/apache/solr/update/UpdateLog.java
##########
@@ -2133,18 +2132,18 @@ public static AddUpdateCommand convertTlogEntryToAddUpdateCommand(SolrQueryReque
new SolrNamedThreadFactory("recoveryExecutor"));
- public static void deleteFile(File file) {
+ public static void deleteFile(Path file) {
boolean success = false;
try {
- Files.deleteIfExists(file.toPath());
+ Files.deleteIfExists(file);
success = true;
} catch (Exception e) {
log.error("Error deleting file: {}", file, e);
}
if (!success) {
try {
- file.deleteOnExit();
+ file.toFile().deleteOnExit();
Review comment:
There's not a deleteOnExit API for NIO. The options are to either convert to a File or register our own shutdown hooks (which is what this does behind the scenes). I think this code exists specifically for Windows running with an AV (or the test FileSystem that simulates this) so I'm not sure what the other replacement options are.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob merged pull request #544: SOLR-15919 Another pass as moving File to Path
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #544:
URL: https://github.com/apache/solr/pull/544
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob merged pull request #544: SOLR-15919 Another pass as moving File to Path
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #544:
URL: https://github.com/apache/solr/pull/544
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] sonatype-lift[bot] commented on a change in pull request #544: SOLR-15919 Another pass as moving File to Path
Posted by GitBox <gi...@apache.org>.
sonatype-lift[bot] commented on a change in pull request #544:
URL: https://github.com/apache/solr/pull/544#discussion_r788332222
##########
File path: solr/core/src/java/org/apache/solr/rest/ManagedResourceStorage.java
##########
@@ -119,20 +120,20 @@ public static StorageIO newStorageIO(String collection, SolrResourceLoader resou
if (storageIO instanceof FileStorageIO) {
// using local fs, if storageDir is not set in the solrconfig.xml, assume the configDir for the core
if (initArgs.get(STORAGE_DIR_INIT_ARG) == null) {
- File configDir = new File(resourceLoader.getConfigDir());
+ Path configDir = resourceLoader.getConfigPath();
boolean hasAccess = false;
try {
- hasAccess = configDir.isDirectory() && configDir.canWrite();
- } catch (java.security.AccessControlException ace) {}
+ hasAccess = Files.isDirectory(configDir) && Files.isWritable(configDir);
+ } catch (SecurityException noAccess) {}
Review comment:
*EmptyCatch:* Caught exceptions should not be ignored [(details)](https://google.github.io/styleguide/javaguide.html#s6.2-caught-exceptions)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
##########
File path: solr/core/src/java/org/apache/solr/handler/admin/ShowFileRequestHandler.java
##########
@@ -350,15 +352,15 @@ public static String getAdminFileFromZooKeeper(SolrQueryRequest req, SolrQueryRe
// Find the file indicated by the "file=XXX" parameter or the root of the conf directory on the local
// file system. Respects all the "interesting" stuff around what the resource loader does to find files.
- public static File getAdminFileFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp,
+ public static Path getAdminFileFromFileSystem(SolrQueryRequest req, SolrQueryResponse rsp,
Set<String> hiddenFiles) {
File adminFile = null;
Review comment:
*UnusedVariable:* The local variable 'adminFile' is never read. [(details)](https://errorprone.info/bugpattern/UnusedVariable)
(at-me [in a reply](https://help.sonatype.com/lift/talking-to-lift) with `help` or `ignore`)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] janhoy commented on a change in pull request #544: SOLR-15919 Another pass as moving File to Path
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #544:
URL: https://github.com/apache/solr/pull/544#discussion_r788243545
##########
File path: solr/core/src/java/org/apache/solr/update/UpdateLog.java
##########
@@ -372,28 +373,34 @@ public void init(UpdateHandler uhandler, SolrCore core) {
return;
}
lastDataDir = dataDir;
- tlogDir = new File(dataDir, TLOG_NAME);
- tlogDir.mkdirs();
- tlogFiles = getLogList(tlogDir);
+ tlogDir = Path.of(dataDir, TLOG_NAME);
+ try {
+ Files.createDirectories(tlogDir);
+ } catch (IOException e) {
+ throw new SolrException(ErrorCode.SERVER_ERROR, "Could not set up tlogs", e);
+ }
+ tlogFiles = getLogList(tlogDir.toFile());
id = getLastLogId() + 1; // add 1 since we will create a new log for the next update
if (debug) {
log.debug("UpdateHandler init: tlogDir={}, existing tlogs={}, next id={}", tlogDir, Arrays.asList(tlogFiles), id);
}
- String[] oldBufferTlog = getBufferLogList(tlogDir);
- if (oldBufferTlog != null && oldBufferTlog.length != 0) {
- existOldBufferLog = true;
+ final String prefix = BUFFER_TLOG_NAME + '.';
+ try (Stream<Path> bufferedTLogs = Files.walk(tlogDir, 1)) {
+ existOldBufferLog = bufferedTLogs.anyMatch(path -> path.getFileName().toString().startsWith(prefix));
+ } catch (IOException e) {
+ // TODO
Review comment:
TODO
##########
File path: solr/core/src/java/org/apache/solr/update/UpdateLog.java
##########
@@ -1328,25 +1325,27 @@ public void copyOverOldUpdates(long commitVersion, TransactionLog oldTlog) {
protected void ensureBufferTlog() {
if (bufferTlog != null) return;
String newLogName = String.format(Locale.ROOT, LOG_FILENAME_PATTERN, BUFFER_TLOG_NAME, System.nanoTime());
- bufferTlog = newTransactionLog(new File(tlogDir, newLogName), globalStrings, false);
+ bufferTlog = newTransactionLog(tlogDir.resolve(newLogName), globalStrings, false);
bufferTlog.isBuffer = true;
}
// Cleanup old buffer tlogs
protected void deleteBufferLogs() {
- String[] oldBufferTlog = getBufferLogList(tlogDir);
- if (oldBufferTlog != null && oldBufferTlog.length != 0) {
- for (String oldBufferLogName : oldBufferTlog) {
- deleteFile(new File(tlogDir, oldBufferLogName));
- }
+ try (Stream<Path> tlogs = Files.walk(tlogDir, 1)) {
+ final String prefix = BUFFER_TLOG_NAME + '.';
+ tlogs.filter(Files::isRegularFile)
+ .filter(path -> path.getFileName().toString().startsWith(prefix))
+ .forEach(UpdateLog::deleteFile);
+ } catch (IOException e) {
+ log.warn("Could not clean up buffered transaction logs in {}", tlogDir, e);
Review comment:
Nice that you also add more informative exception messages
##########
File path: solr/core/src/java/org/apache/solr/update/UpdateLog.java
##########
@@ -2133,18 +2132,18 @@ public static AddUpdateCommand convertTlogEntryToAddUpdateCommand(SolrQueryReque
new SolrNamedThreadFactory("recoveryExecutor"));
- public static void deleteFile(File file) {
+ public static void deleteFile(Path file) {
boolean success = false;
try {
- Files.deleteIfExists(file.toPath());
+ Files.deleteIfExists(file);
success = true;
} catch (Exception e) {
log.error("Error deleting file: {}", file, e);
}
if (!success) {
try {
- file.deleteOnExit();
+ file.toFile().deleteOnExit();
Review comment:
Just looks a bit weird with `file.toFile()`, but probably ok. Have not checked if there is a deleteOnExit API on Files..
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org