SOLR-16553 and SOLR-16559 #1185
Conversation
| @@ -476,18 +479,22 @@ private void addFoldersToSharedLib(Set<String> libDirs) { | |||
|
|
|||
| // Adds modules to shared classpath | |||
| private void initModules() { | |||
| Path solrInstallDir = getSolrInstallDir(); | |||
| if (solrInstallDir == null) { | |||
| return; | |||
There was a problem hiding this comment.
I'm not a fan of returning null to handle exceptional cases. Why not catch the exception instead?
And certainly not a fan of silenlty returning like this. At the very least, it should do a check to see if any modules are requested, and throw exception if they are but we cannot load them due to, in this case, a configuration error.
There was a problem hiding this comment.
I don't think getSolrInstallDir() should return an exception if the property isn't set. Returning null from that method is 100% valid since it is saying "there is no install dir" - initModules and setupSharedLib should handle the case of no install dir. I agree that there should be a message returned in that case if there is no install dir.
There was a problem hiding this comment.
@janhoy I pushed a change that gives some more detail in the case of modules and shared lib. Let me know if that looks better for you.
There was a problem hiding this comment.
This was better, although we'd probably be better off for all tests to also set solr.install.dir and treat lack thereof as a configuration error. But now we at least report why things are failing if it is not set. I'm ok with this too.
Contains two commits that are related to fixing the benchmark stuff.