SOLR-15950: Fix auto-creation of filestore in constructor.#653
Conversation
|
I can confirm this at least makes Checked on main again and on this pr - passes with changes from this PR. |
|
Opened a separate PR for the test cleanup: #654 |
| try { | ||
| Files.createDirectories(path); | ||
| log.info("Created filestore folder {}", path); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
Should we catch AccessControlException here too?
There was a problem hiding this comment.
I'm ambivalent, it'll be thrown anyways and honestly shouldn't happen in a real deployment. Feel free to add it.
There was a problem hiding this 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
If we need to check for AccessControlException here, we'd likely need the same in 100 other locations across our code base?
There was a problem hiding this 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.
|
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. |
|
I would also expect |
|
The filestore is its own feature that could be used by something other than packages. |
Is the |
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? |
janhoy
left a comment
janhoy
left a comment
There was a problem hiding this comment.
I checked out the branch, started Solr on empty SOLR_HOME, looks good.
(cherry picked from commit 03efd00)
(cherry picked from commit 03efd00)
5 participants
https://issues.apache.org/jira/browse/SOLR-15950
The filestore lazy-creation change made previously didn't actually make it lazy, since the directory is ensured in the constructor of the class. By merely removing that constructor call/method, it is now lazy.
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.