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/02/18 18:21:46 UTC
[GitHub] [solr] HoustonPutman opened a new pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
HoustonPutman opened a new pull request #653:
URL: https://github.com/apache/solr/pull/653
https://issues.apache.org/jira/browse/SOLR-15950
Note that I am also removing the need to create a SolrCore in the Managed Resource test. This is completely unrelated, but I saw that it wasn't needed when debugging why the test was failing. Can split this into a separate PR.
--
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] HoustonPutman commented on pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1045002903
Opened a separate PR for the test cleanup: #654
--
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] dsmiley commented on pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1045789532
The filestore is its own feature that could be used by something other than packages.
--
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] HoustonPutman commented on a change in pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #653:
URL: https://github.com/apache/solr/pull/653#discussion_r810322533
##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -563,28 +562,14 @@ public static boolean isMetaDataFile(String file) {
return file.charAt(0) == '.' && file.endsWith(".json");
}
- private void ensurePackageStoreDir(Path solrHome) {
- final File packageStoreDir = getPackageStoreDirPath(solrHome).toFile();
- if (!packageStoreDir.exists()) {
- try {
- final boolean created = packageStoreDir.mkdirs();
- if (!created) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome);
- }
- } catch (Exception e) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome, e);
- }
- }
- }
-
public static synchronized Path getPackageStoreDirPath(Path solrHome) {
var path = solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
if (!Files.exists(path)) {
try {
Files.createDirectories(path);
log.info("Created filestore folder {}", path);
} catch (IOException e) {
Review comment:
I'm ambivalent, it'll be thrown anyways and honestly shouldn't happen in a real deployment. Feel free to add it.
--
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 #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #653:
URL: https://github.com/apache/solr/pull/653#discussion_r811249550
##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -563,28 +562,14 @@ public static boolean isMetaDataFile(String file) {
return file.charAt(0) == '.' && file.endsWith(".json");
}
- private void ensurePackageStoreDir(Path solrHome) {
- final File packageStoreDir = getPackageStoreDirPath(solrHome).toFile();
- if (!packageStoreDir.exists()) {
- try {
- final boolean created = packageStoreDir.mkdirs();
- if (!created) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome);
- }
- } catch (Exception e) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome, e);
- }
- }
- }
-
public static synchronized Path getPackageStoreDirPath(Path solrHome) {
var path = solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
if (!Files.exists(path)) {
try {
Files.createDirectories(path);
log.info("Created filestore folder {}", path);
} catch (IOException e) {
Review comment:
The policy already have this line
```
permission java.io.FilePermission "${solr.solr.home}", "read,write,delete,readlink";
```
So that should be allowed -- if sysProp `solr.solr.home` is set to the same location as `solrHome` in the test above that is...
--
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 #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #653:
URL: https://github.com/apache/solr/pull/653#discussion_r810304779
##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -563,28 +562,14 @@ public static boolean isMetaDataFile(String file) {
return file.charAt(0) == '.' && file.endsWith(".json");
}
- private void ensurePackageStoreDir(Path solrHome) {
- final File packageStoreDir = getPackageStoreDirPath(solrHome).toFile();
- if (!packageStoreDir.exists()) {
- try {
- final boolean created = packageStoreDir.mkdirs();
- if (!created) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome);
- }
- } catch (Exception e) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome, e);
- }
- }
- }
-
public static synchronized Path getPackageStoreDirPath(Path solrHome) {
var path = solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
if (!Files.exists(path)) {
try {
Files.createDirectories(path);
log.info("Created filestore folder {}", path);
} catch (IOException e) {
Review comment:
Should we catch `AccessControlException` here too?
--
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] risdenk commented on pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
risdenk commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1044977897
I can confirm this at least makes `org.apache.solr.rest.TestManagedResourceStorage` pass which has been failing on main.
```
./gradlew :solr:core:test --tests "org.apache.solr.rest.TestManagedResourceStorage" -Ptests.jvmargs=-XX:TieredStopAtLevel=1 -Ptests.seed=8833B2251FFB9A75 -Ptests.badapples=false -Ptests.file.encoding=UTF-8
```
Checked on main again and on this pr - passes with changes from this PR.
--
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] HoustonPutman merged pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #653:
URL: https://github.com/apache/solr/pull/653
--
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 #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #653:
URL: https://github.com/apache/solr/pull/653#discussion_r811247161
##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -563,28 +562,14 @@ public static boolean isMetaDataFile(String file) {
return file.charAt(0) == '.' && file.endsWith(".json");
}
- private void ensurePackageStoreDir(Path solrHome) {
- final File packageStoreDir = getPackageStoreDirPath(solrHome).toFile();
- if (!packageStoreDir.exists()) {
- try {
- final boolean created = packageStoreDir.mkdirs();
- if (!created) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome);
- }
- } catch (Exception e) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome, e);
- }
- }
- }
-
public static synchronized Path getPackageStoreDirPath(Path solrHome) {
var path = solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
if (!Files.exists(path)) {
try {
Files.createDirectories(path);
log.info("Created filestore folder {}", path);
} catch (IOException e) {
Review comment:
It is thrown by Security Manager, right? Which is default enabled in 9.0 (but deprecated in JDK17). The path that was denied was
```
java.security.AccessControlException: access denied ("java.io.FilePermission" "/home/jenkins/jenkins-slave/workspace/Solr/Solr-Check-main/solr/core/build/resources/test/solr/filestore" "write")
```
So obviously our code tried to create that folder in the "build/resources/test/solr/" folder, which must have been disallowed by our [solr-tests.policy](https://github.com/apache/solr/blob/main/gradle/testing/randomization/policies/solr-tests.policy)
If we need to check for `AccessControlException` here, we'd likely need the same in 100 other locations across our code base?
--
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 pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1046054117
> The filestore is its own feature that could be used by something other than packages.
Is the `DistribPackageStore` class ill named and should have been `DistributedFileStore`?
--
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 pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1046849214
> > Is the DistribPackageStore class ill named and should have been DistributedFileStore
>
> Yes absolutely (including the interface PackageStore). The re-imagination of the distributed package store as the general purpose file store came relatively late in the new package system design, so the rename didn't happen yet. Now would be a great time to make this change.
>
> CC @noblepaul
Spun this off to https://issues.apache.org/jira/browse/SOLR-16038 - @noblepaul do you want to pick that up?
Is this PR ready?
--
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 #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
janhoy commented on a change in pull request #653:
URL: https://github.com/apache/solr/pull/653#discussion_r811249550
##########
File path: solr/core/src/java/org/apache/solr/filestore/DistribPackageStore.java
##########
@@ -563,28 +562,14 @@ public static boolean isMetaDataFile(String file) {
return file.charAt(0) == '.' && file.endsWith(".json");
}
- private void ensurePackageStoreDir(Path solrHome) {
- final File packageStoreDir = getPackageStoreDirPath(solrHome).toFile();
- if (!packageStoreDir.exists()) {
- try {
- final boolean created = packageStoreDir.mkdirs();
- if (!created) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome);
- }
- } catch (Exception e) {
- log.warn("Unable to create [{}] directory in SOLR_HOME [{}]. Features requiring this directory may fail.", packageStoreDir, solrHome, e);
- }
- }
- }
-
public static synchronized Path getPackageStoreDirPath(Path solrHome) {
var path = solrHome.resolve(PackageStoreAPI.PACKAGESTORE_DIRECTORY);
if (!Files.exists(path)) {
try {
Files.createDirectories(path);
log.info("Created filestore folder {}", path);
} catch (IOException e) {
Review comment:
The policy already have this line
```
permission java.io.FilePermission "${solr.solr.home}", "read,write,delete,readlink";
```
So that should be allowed -- if sysProp `solr.solr.home` is set to the same location as `solrHome` in the test that failed (`TestManagedResourceStorage`) that is, and I guess it is not...
Anyway, this PR avoids that now.
--
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 pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1045375820
I started looking into it but reverted original commit on main. Then I learned about this PR, which is of course now broken. I'll un-revert again to enable this PR as a solution instead.
--
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 pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
janhoy commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1045389490
I would also expect `enable.packages=false` to avoid loading `DistribPackageStore` completely, but it may be needed for other features than pacakge loading?
--
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] dsmiley commented on pull request #653: SOLR-15950: Fix auto-creation of filestore in constructor.
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #653:
URL: https://github.com/apache/solr/pull/653#issuecomment-1046057422
> Is the DistribPackageStore class ill named and should have been DistributedFileStore
Yes absolutely (including the interface PackageStore). The re-imagination of the distributed package store as the general purpose file store came relatively late in the new package system design, so the rename didn't happen yet. Now would be a great time to make this change.
CC @noblepaul
--
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