Problem/Motivation

INSTALL.md file contains misleading and outdated instructions which leads to bad practices treated as a good one.

Examples:

  1. creating Solr core manually - Solr have script to create a core bundled with it since ages, at least since version 5.0
  2. configuring solr.install.dir in solrcore.properties - solr.install.dir is a system property that was introduced in v4.10.3 https://lucene.apache.org/solr/8_0_0/changes/Changes.html#v4.10.3.other_.... This is set by Solr start up script (bin/solr) and should not be overridden from solrcore.properties

Proposed resolution

Remove installation instructions from INSTALL.md and replace them with links to the Solr documentation portal:

  1. https://lucene.apache.org/solr/guide/8_2/installing-solr.html
  2. https://lucene.apache.org/solr/guide/8_2/taking-solr-to-production.html
  3. https://lucene.apache.org/solr/guide/ - for a list of other version specific guides

Update instructions on how to create a Solr core.

Remove solr.install.dir from solrcore.properties in the generated files and in the module UI.

Remaining tasks

  1. Agree on proposed resolution
  2. Create a patch
  3. Review
  4. Commit
  5. Update documentation on Drupal.org where applicable

User interface changes

Removal of solr.install.dir input in configuration form.

API changes

None.

Data model changes

None.

Release notes snippet

TDB

CommentFileSizeAuthor
#7 3100457.patch14.53 KBmkalkbrenner
#2 search_api_solr-3100457-2.patch18.81 KBRoSk0
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RoSk0 created an issue. See original summary.

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 at Catalyst IT commented
Status: Active » Needs review
FileSize
search_api_solr-3100457-2.patch18.81 KB

Initial patch.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Needs work

First of all you should fiele a PR on github to get your patch tested.

And I'm not entirely happy with that patch. But let's discuss that after having the test results of travis.

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 at Catalyst IT commented

PR tested successfully.
https://github.com/mkalkbrenner/search_api_solr/pull/52

RoSk0’s picture

RoSk0
he/him
Primary language Russian
Location Wellington
CreditAttribution: RoSk0 at Catalyst IT commented
Status: Needs work » Needs review

Can we discuss this now?

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
Title: Remove mislieading configuration instructions » Don't force solr.install.dir to be configured in solrcore.properties if Solr is properly installed and started
Assigned: Unassigned » mkalkbrenner 
+++ b/solr-conf-templates/6.x/solrconfig.xml
@@ -73,17 +73,17 @@
-  <lib dir="${solr.install.dir:../../../..}/dist/" regex="solr-dataimporthandler-.*\.jar" />
+  <lib dir="${solr.install.dir}/dist/" regex="solr-dataimporthandler-.*\.jar" />

I don't agree with this change! Even the latest Solr examples set the path like this. See
https://github.com/apache/lucene-solr/blob/branch_8_4/solr/example/examp...

But in genaral I agree that setting solr.install.dir in solrcore.properties should be avoided nowadays. But dropping it entirely isn't the correct solution. Otherwise we'll loose features like the support for different Solr servers on the same machine.

I propose to remove the solr.install.dir property from solrcore.properties if it is set to an empty string in the config.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
FileSize
3100457.patch14.53 KB

An interdiff isn't that readable. Therefore I just upload a new patch.

  • mkalkbrenner committed f0ee0b2 on 8.x-3.x 
    Issue #3100457 by RoSk0, mkalkbrenner: Don't force solr.install.dir to...
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.