[GitHub] [solr] uschindler commented on a diff in pull request #1360: SOLR 16642 : upgrade Solr to use Lucene 9.5.0

["uschindler (via GitHub)" <gi...@apache.org>]

uschindler commented on code in PR #1360:
URL: https://github.com/apache/solr/pull/1360#discussion_r1122054880


##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
       throws IOException {
     MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
     try {
-      mapDirectory.setUseUnmap(unmapHack);
+      System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));

Review Comment:
   If somebody wants to use this, starting with Lucene 9.5, you have to pass this sysprop in `solr.in.sh`.
   
   I was not aware that this setting was still available in Solr's configs.... oh man oh man. When deprecating it I checked Github search to find usages. I found it in older Solr versions but no third party code. Also nowhere in user's solrconfig.xml I found any instance of this flag.
   
   So maybe just ignore it an log a warning that you need to pass the setting in Solr's startup script to have an effect. The current implementation won't work at all, because whe this DirectoryFactory is loaded, it implicitely loads MMapDirectory. This causes static initializer to run and the setting is read from sysprop. Changing it later has no effect anymore.



##########
solr/core/src/java/org/apache/solr/core/MMapDirectoryFactory.java:
##########
@@ -63,7 +63,7 @@ protected Directory create(String path, LockFactory lockFactory, DirContext dirC
       throws IOException {
     MMapDirectory mapDirectory = new MMapDirectory(Path.of(path), lockFactory, maxChunk);
     try {
-      mapDirectory.setUseUnmap(unmapHack);
+      System.setProperty(MMapDirectory.ENABLE_UNMAP_HACK_SYSPROP, String.valueOf(unmapHack));

Review Comment:
   this wont work at all, because it is not dynamic. It only works if it is populated at startup time, because the property is read on early booting. Remove this completely.
   
   You can still keep the setting but make it throw UOE. That is what happens when you call the deprecated method on Lucene 9.5.



-- 
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