epugh commented on code in PR #2907:
URL: https://github.com/apache/solr/pull/2907#discussion_r1884509931
##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -294,7 +294,7 @@ public void testDoFilesMode() throws IOException {
postTool.recursive = 0;
postTool.dryRun = true;
postTool.solrUpdateUrl =
URI.create("http://localhost:8983/solr/fake/update";);
- File dir = getFile("exampledocs");
+ File dir = getFile("exampledocs").toFile();
Review Comment:
Since we just do a toString, maybe we can simplify this line???
##########
solr/core/src/java/org/apache/solr/cloud/SolrZkServer.java:
##########
@@ -277,8 +276,8 @@ public static boolean hasServers(Properties props) {
return false;
}
- public void setDataDir(File dataDir) {
- this.dataDir = dataDir;
+ public void setDataDir(Path dataDir) {
+ this.dataDir = dataDir.toFile();
Review Comment:
is this an example of where you felt changing things got out of hand?
Otherwise, seems like if `dataDir` was a Path, then no `.toFile()`??
##########
solr/benchmark/src/java/org/apache/solr/bench/MiniClusterState.java:
##########
@@ -267,7 +267,7 @@ public void startMiniCluster(int nodeCount) {
cluster =
new MiniSolrCloudCluster.Builder(nodeCount, miniClusterBaseDir)
.formatZkServer(false)
- .addConfig("conf",
getFile("src/resources/configs/cloud-minimal/conf").toPath())
+ .addConfig("conf",
getFile("src/resources/configs/cloud-minimal/conf"))
Review Comment:
love seeing the changed version is shorter and simpler than the original one!
##########
solr/modules/scripting/src/test/org/apache/solr/scripting/update/ScriptUpdateProcessorFactoryTest.java:
##########
@@ -44,7 +44,7 @@ public static void beforeClass() throws Exception {
initCore(
"solrconfig-script-updateprocessor.xml",
"schema.xml",
- getFile("scripting/solr").getAbsolutePath());
+ getFile("scripting/solr").toAbsolutePath().toString());
Review Comment:
Maybe better to change `initCore`?
##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -294,7 +294,7 @@ public void testDoFilesMode() throws IOException {
postTool.recursive = 0;
postTool.dryRun = true;
postTool.solrUpdateUrl =
URI.create("http://localhost:8983/solr/fake/update";);
- File dir = getFile("exampledocs");
+ File dir = getFile("exampledocs").toFile();
Review Comment:
matbe change postFiles to take in a list of paths?
##########
solr/core/src/test/org/apache/solr/search/TestRecovery.java:
##########
Review Comment:
This feels like maybe an opportunity to change from using either Strings and
Files to just path and avoid some conversions??
##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -303,7 +303,7 @@ public void testDoFilesMode() throws IOException {
public void testDetectingIfRecursionPossibleInFilesMode() throws IOException
{
PostTool postTool = new PostTool();
postTool.recursive = 1; // This is the default
- File dir = getFile("exampledocs");
+ File dir = getFile("exampledocs").toFile();
Review Comment:
likewise, maybe we just make strings and avoid going thoruh the file type?
##########
solr/core/src/java/org/apache/solr/core/DirectoryFactory.java:
##########
@@ -339,6 +339,8 @@ public String getDataHome(CoreDescriptor cd) throws
IOException {
public void cleanupOldIndexDirectories(
final String dataDirPath, final String currentIndexDirPath, boolean
afterCoreReload) {
+
+ // TODO SOLR-8282 move to PATH
Review Comment:
Your stepwise logic is probably much better than a "lets change it all in
one fell swoop!"
##########
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudHttp2SolrClientRetryTest.java:
##########
@@ -41,7 +41,6 @@ public static void setupCluster() throws Exception {
.addConfig(
"conf",
getFile("solrj")
- .toPath()
Review Comment:
Sweet!
##########
solr/cross-dc-manager/src/test/org/apache/solr/crossdc/manager/RetryQueueIntegrationTest.java:
##########
@@ -157,7 +157,7 @@ private static MiniSolrCloudCluster startCluster(
// new SolrCloudTestCase.Builder(1, baseDir).addConfig("conf",
// getFile("configs/cloud-minimal/conf").toPath()).configure();
- cluster.uploadConfigSet(getFile("configs/cloud-minimal/conf").toPath(),
"conf");
+ cluster.uploadConfigSet(getFile("configs/cloud-minimal/conf"), "conf");
Review Comment:
Love it!
##########
solr/core/src/test/org/apache/solr/cli/PostToolTest.java:
##########
@@ -303,7 +303,7 @@ public void testDoFilesMode() throws IOException {
public void testDetectingIfRecursionPossibleInFilesMode() throws IOException
{
PostTool postTool = new PostTool();
postTool.recursive = 1; // This is the default
- File dir = getFile("exampledocs");
+ File dir = getFile("exampledocs").toFile();
Review Comment:
Or stay as a Path?
--
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]
