Solr
  1. Solr
  2. SOLR-2983

Unable to load custom MergePolicy

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.6, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None

      Description

      As part of a recent upgrade to Solr 3.5.0 we encountered an error related to our use of LinkedIn's ZoieMergePolicy.

      It seems the code that loads a custom MergePolicy was at some point moved into SolrIndexConfig.java from SolrIndexWriter.java, but as this code was copied verbatim it now contains a bug:

      try {
      policy = (MergePolicy) schema.getResourceLoader().newInstance(mpClassName, null, new Class[]

      {IndexWriter.class}

      , new Object[]

      {this}

      );
      } catch (Exception e)

      { policy = (MergePolicy) schema.getResourceLoader().newInstance(mpClassName); }

      'this' is no longer an IndexWriter but a SolrIndexConfig, therefore the call to newInstance will always throw an exception and the catch clause will be executed. If the custom MergePolicy does not have a default constructor (which is the case of ZoieMergePolicy), the second attempt to create the MergePolicy will also fail and Solr won't start.

      1. SOLR-2983_2.patch
        8 kB
        Tommaso Teofili
      2. SOLR-2983.patch
        7 kB
        Tommaso Teofili

        Activity

        Hide
        Hoss Man added a comment -

        this seems like a really silly and anoying mistake in 3.5 ... we clearly need better tests using mock MergePolicies.

        herbert: do you have a patch to fix the init code?

        Show
        Hoss Man added a comment - this seems like a really silly and anoying mistake in 3.5 ... we clearly need better tests using mock MergePolicies. herbert: do you have a patch to fix the init code?
        Hide
        Mathias Herberts added a comment -

        IIRC the IndexWriter is not available in SolrIndexConfig, so I don't have an easy fix.

        Show
        Mathias Herberts added a comment - IIRC the IndexWriter is not available in SolrIndexConfig, so I don't have an easy fix.
        Hide
        Tommaso Teofili added a comment -

        it looks like it's a cyclic dependency problem since the SolrIndexWriter uses the SolrIndexConfig.toIndexWriterConfig method to create an IndexWriterConfig which is used to call the basic Lucene IndexWriter constructor while at the same time the SolrIndexConfig.toIndexWriterConfig may need an IndexWriter to instantiate the MergePolicy (try clause).

        Show
        Tommaso Teofili added a comment - it looks like it's a cyclic dependency problem since the SolrIndexWriter uses the SolrIndexConfig.toIndexWriterConfig method to create an IndexWriterConfig which is used to call the basic Lucene IndexWriter constructor while at the same time the SolrIndexConfig.toIndexWriterConfig may need an IndexWriter to instantiate the MergePolicy (try clause).
        Hide
        Simon Willnauer added a comment -

        honestly expecting IW in the ctor is bogus. IndexWriter passes itself to the mergePolicy in MergePolicy#setIndexWriter() and we should really keep it that way. the fix here is to not give the option to pass IW to the MP in the ctor.

        Show
        Simon Willnauer added a comment - honestly expecting IW in the ctor is bogus. IndexWriter passes itself to the mergePolicy in MergePolicy#setIndexWriter() and we should really keep it that way. the fix here is to not give the option to pass IW to the MP in the ctor.
        Hide
        Tommaso Teofili added a comment -

        that probably depends on migrations from old APIs, however, apart from that, I agree the setter (and SetOnce) facilities are the best way to inject an IW in the mergePolicy.
        Therefore the above try/catch clause has little meaning and IMHO it may be better to just keep the policy instantiation like this:

        MergePolicy policy = (MergePolicy) schema.getResourceLoader().newInstance(mpClassName);

        Show
        Tommaso Teofili added a comment - that probably depends on migrations from old APIs, however, apart from that, I agree the setter (and SetOnce) facilities are the best way to inject an IW in the mergePolicy. Therefore the above try/catch clause has little meaning and IMHO it may be better to just keep the policy instantiation like this: MergePolicy policy = (MergePolicy) schema.getResourceLoader().newInstance(mpClassName);
        Hide
        Tommaso Teofili added a comment -

        simple patch which just removes the (always failing) try clause, adding unit tests for a bad merge policy sample

        Show
        Tommaso Teofili added a comment - simple patch which just removes the (always failing) try clause, adding unit tests for a bad merge policy sample
        Hide
        Simon Willnauer added a comment -

        I agree we should just get rid of it. I think this deserves a CHANGES.TXT entry.

        Show
        Simon Willnauer added a comment - I agree we should just get rid of it. I think this deserves a CHANGES.TXT entry.
        Hide
        Simon Willnauer added a comment -

        tomasso can you update changes.txt too. once this is done I can just commit it, thanks!

        Show
        Simon Willnauer added a comment - tomasso can you update changes.txt too. once this is done I can just commit it, thanks!
        Hide
        Hoss Man added a comment -

        Tomas: as simon said, this patch looks good provided we have a blurb about the change in the upgrade/backcompat section.

        wanna go ahead an commit for 3.6?

        (although: please remove that cut/paste comment about "kitchen sink" in your new bad-mp-solrconfig.xml)

        Show
        Hoss Man added a comment - Tomas: as simon said, this patch looks good provided we have a blurb about the change in the upgrade/backcompat section. wanna go ahead an commit for 3.6? (although: please remove that cut/paste comment about "kitchen sink" in your new bad-mp-solrconfig.xml)
        Hide
        Tomás Fernández Löbbe added a comment -

        I think it was assigned to the wrong person. Assigning it to Tommaso

        Show
        Tomás Fernández Löbbe added a comment - I think it was assigned to the wrong person. Assigning it to Tommaso
        Hide
        Tommaso Teofili added a comment -

        Agreed, I'm going to do the needed updates to changes.txt and upgrade/backcompat.

        Show
        Tommaso Teofili added a comment - Agreed, I'm going to do the needed updates to changes.txt and upgrade/backcompat.
        Hide
        Tommaso Teofili added a comment -

        I just noticed also the toIndexWriter method should be explicitly tested, going to work on it and attach a new patch

        Show
        Tommaso Teofili added a comment - I just noticed also the toIndexWriter method should be explicitly tested, going to work on it and attach a new patch
        Hide
        Tommaso Teofili added a comment -

        new patch which adds the changes.txt entry and adds the toIndexWriterConfig() method (the one which caused the failures) testing

        Show
        Tommaso Teofili added a comment - new patch which adds the changes.txt entry and adds the toIndexWriterConfig() method (the one which caused the failures) testing
        Hide
        Tommaso Teofili added a comment -

        committed on trunk at r1304098

        Show
        Tommaso Teofili added a comment - committed on trunk at r1304098
        Hide
        Tommaso Teofili added a comment - - edited

        and merged to branch_3x on r1306733

        Show
        Tommaso Teofili added a comment - - edited and merged to branch_3x on r1306733
        Hide
        Tommaso Teofili added a comment -

        SolrIndexConfigTest fails in 3x because the SolrIndexConfig constructor "prefix" parameter cannot be null

        Show
        Tommaso Teofili added a comment - SolrIndexConfigTest fails in 3x because the SolrIndexConfig constructor "prefix" parameter cannot be null
        Hide
        Tommaso Teofili added a comment -

        fixed issue with SolrIndexConfigTest in branch_3x

        Show
        Tommaso Teofili added a comment - fixed issue with SolrIndexConfigTest in branch_3x

          People

          • Assignee:
            Tommaso Teofili
            Reporter:
            Mathias Herberts
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development