mlbiscoc commented on code in PR #2924:
URL: https://github.com/apache/solr/pull/2924#discussion_r1937718233
##########
solr/core/src/java/org/apache/solr/core/SolrPaths.java:
##########
@@ -94,7 +94,7 @@ public static void assertNoPathTraversal(Path pathToAssert) {
/** Asserts that a path is not a Windows UNC path */
public static void assertNotUnc(Path pathToAssert) {
- if (OS.isFamilyWindows() && pathToAssert.toString().startsWith("\\\\")) {
+ if (OS.isFamilyWindows() || pathToAssert.toString().startsWith("\\\\")) {
Review Comment:
Thats true but currently the assertion is skipped on non-windows even if the
path is a UNC path because of the AND when I thought this function was to only
check UNC but maybe that was for a reason.
I'm going to revert this and revert `openResource` back to its original
assertion.
##########
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##########
@@ -1354,16 +1353,18 @@ public void initializeMetrics(SolrMetricsContext
parentContext, String scope) {
Category.CORE.toString());
}
- // TODO SOLR-8282 move to PATH
// initialize disk total / free metrics
- Path dataDirPath = Paths.get(dataDir);
- File dataDirFile = dataDirPath.toFile();
- parentContext.gauge(
- () -> dataDirFile.getTotalSpace(), true, "totalSpace",
Category.CORE.toString(), "fs");
- parentContext.gauge(
- () -> dataDirFile.getUsableSpace(), true, "usableSpace",
Category.CORE.toString(), "fs");
+ Path dataDirFile = Paths.get(dataDir);
+
+ // Do not pre-compute the data directory total/usable space on
initialization. Calculated when
+ // metric is fetched
+ // TODO Move metrics initialization calls to after the data directory is
created to fetch true
Review Comment:
I had a misunderstanding of how this initialization of gauges worked. I
changed it to what I think should be the correct conversion to Path and seems
to work testing.
##########
solr/core/src/java/org/apache/solr/handler/admin/CoreAdminOperation.java:
##########
@@ -357,7 +356,7 @@ public static NamedList<Object> getCoreStatus(
if (core != null) {
info.add(NAME, core.getName());
info.add("instanceDir", core.getInstancePath().toString());
- info.add("dataDir", normalizePath(core.getDataDir()));
+ info.add("dataDir", Path.of(core.getDataDir()).toString());
Review Comment:
Found a place in `FileUtils` and brought it back. Also just switched to
`FileSystems.getDefault().getSeparator()` and brought all the logic to be
careful of regressions since it seems neither of us can confirm fully on a
windows machine.
--
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-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
