Solr
  1. Solr
  2. SOLR-8621

solrconfig.xml: deprecate/replace <mergePolicy> with <mergePolicyFactory>

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      <mergePolicyFactory> end-user benefits:

      • Lucene's UpgradeIndexMergePolicy can be configured in Solr
      • Lucene's SortingMergePolicy can be configured in Solr (with SOLR-5730)
      • customisability: arbitrary merge policies including wrapping/nested merge policies can be created and configured

      roadmap:

      • solr 5.5 introduces <mergePolicyFactory> support
      • solr 5.5 deprecates (but maintains) <mergePolicy> support
      • SOLR-8668 in solr 6.0(?) will remove <mergePolicy> support
      1. explicit-merge-auto-set.patch
        3 kB
        Shawn Heisey
      2. SOLR-8621.patch
        42 kB
        Shai Erera
      3. SOLR-8621-example_contrib_configs.patch
        15 kB
        Shai Erera
      4. SOLR-8621-example_contrib_configs.patch
        15 kB
        Shai Erera

        Issue Links

          Activity

          Hide
          Christine Poerschke added a comment -

          Shai Erera wrote in SOLR-5730:

          ... How about if we introduce a MergePolicyPlugin/Factory, which would allow one to implement whatever MergePolicy he wants (including wrapping others). ...

          Christine Poerschke wrote in SOLR-5730:

          I like the idea of a MergePolicyFactory ... We currently have mergeFactor and maxMergeDocs elements alongside the mergePolicy element. I would see the MergePolicyFactory route as an opportunity to deprecate the mergeFactor/maxMergeDocs/mergePolicy elements i.e. for a transition period users can use the existing/deprecated elements or the new mergePolicyFactory element but eventually we will remove the deprecated elements. ...

          Show
          Christine Poerschke added a comment - Shai Erera wrote in SOLR-5730 : ... How about if we introduce a MergePolicyPlugin/Factory, which would allow one to implement whatever MergePolicy he wants (including wrapping others). ... Christine Poerschke wrote in SOLR-5730 : I like the idea of a MergePolicyFactory ... We currently have mergeFactor and maxMergeDocs elements alongside the mergePolicy element. I would see the MergePolicyFactory route as an opportunity to deprecate the mergeFactor/maxMergeDocs/mergePolicy elements i.e. for a transition period users can use the existing/deprecated elements or the new mergePolicyFactory element but eventually we will remove the deprecated elements. ...
          Hide
          Christine Poerschke added a comment -

          proposed in SOLR-5730:

          ... out-of-the-box factories for all existing non-wrapping merge policies e.g.

          <!-- public TieredMergePolicy() { -->
          <mergePolicyFactory class="TieredMergePolicyFactory">
            <int name="segmentsPerTier">42</int>
          </mergePolicyFactory>
          

          An out-of-the-box factory ... for existing plain wrapping merge policies e.g.

          <!-- public UpgradeIndexMergePolicy(MergePolicy base) { -->
          <mergePolicyFactory class="UpgradeIndexMergePolicyFactory">
            <str name="base">TieredMergePolicyFactory</str>
            <int name="base.segmentsPerTier">42</int>
          </mergePolicyFactory>
          

          The factory for building sorting merge policies [in SOLR-5730] could look something like this:

          <!-- public SortingMergePolicy(MergePolicy in, Sort sort) { -->
          <mergePolicyFactory class="SortingMergePolicyFactory">
            <str name="in">TieredMergePolicyFactory</str>
            <int name="in.segmentsPerTier">42</int>
            <str name="sort">timestamp desc</str>
          </mergePolicyFactory>
          

          For consistency ... conceptually support wrapping of wrapping merge policies e.g. someone could go and create their own MyInstrumentedMergePolicyFactory factory and use it like this:

          <!-- public MyInstrumentedMergePolicy(MergePolicy mergePolicy) { -->
          <mergePolicyFactory class="MyInstrumentedMergePolicyFactory">
            <str name="mergePolicy">SortingMergePolicyFactory</str>
            <str name="mergePolicy.in">TieredMergePolicyFactory</str>
            <int name="mergePolicy.in.segmentsPerTier">42</int>
            <str name="mergePolicy.sort">timestamp desc</str>
            <int name="instrumentationLevel">42</int>
          </mergePolicyFactory>
          
          Show
          Christine Poerschke added a comment - proposed in SOLR-5730 : ... out-of-the-box factories for all existing non-wrapping merge policies e.g. <!-- public TieredMergePolicy() { --> <mergePolicyFactory class= "TieredMergePolicyFactory" > < int name= "segmentsPerTier" >42</ int > </mergePolicyFactory> An out-of-the-box factory ... for existing plain wrapping merge policies e.g. <!-- public UpgradeIndexMergePolicy(MergePolicy base) { --> <mergePolicyFactory class= "UpgradeIndexMergePolicyFactory" > <str name= "base" >TieredMergePolicyFactory</str> < int name= "base.segmentsPerTier" >42</ int > </mergePolicyFactory> The factory for building sorting merge policies [in SOLR-5730 ] could look something like this: <!-- public SortingMergePolicy(MergePolicy in, Sort sort) { --> <mergePolicyFactory class= "SortingMergePolicyFactory" > <str name= "in" >TieredMergePolicyFactory</str> < int name= "in.segmentsPerTier" >42</ int > <str name= "sort" >timestamp desc</str> </mergePolicyFactory> For consistency ... conceptually support wrapping of wrapping merge policies e.g. someone could go and create their own MyInstrumentedMergePolicyFactory factory and use it like this: <!-- public MyInstrumentedMergePolicy(MergePolicy mergePolicy) { --> <mergePolicyFactory class= "MyInstrumentedMergePolicyFactory" > <str name= "mergePolicy" >SortingMergePolicyFactory</str> <str name= "mergePolicy.in" >TieredMergePolicyFactory</str> < int name= "mergePolicy.in.segmentsPerTier" >42</ int > <str name= "mergePolicy.sort" >timestamp desc</str> < int name= "instrumentationLevel" >42</ int > </mergePolicyFactory>
          Hide
          ASF subversion and git services added a comment -

          Commit 3c4929e8efa041d53117e6f3669485d96839c98d in lucene-solr's branch refs/heads/master-solr-8621 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3c4929e ]

          [outline] SOLR-8621: solrconfig.xml: deprecate/replace <mergePolicy> with <mergePolicyFactory>

          outline summary:

          subtleties:

          • We currently have mergeFactor and maxMergeDocs elements alongside the mergePolicy element. The mergeFactor/maxMergeDocs/mergePolicy elements will be deprecated together i.e. mergePolicyFactory will ignore mergeFactor and maxMergeDocs. (Note that maxMergeDocs only applies to LogMergePolicy policies, and that mergeFactor only applies to LogMergePolicy and TieredMergePolicy policies.)
          • We currently have SolrIndexConfig.fixUseCFMergePolicyInitArg to account for any <useCompoundFile> in <mergePolicy> (since ?.? <useCompoundFile> is directly specified in <indexConfig>). For the <mergePolicyFactory> element no such accommodations would be made.

          independent:

          • some invokeSetter functionality from solr/util/SolrPluginUtils factored out into lucene/util/LucenePluginUtils (SolrPluginUtilsTest would need similar factoring out)

          lucene:

          • new classes (but no tests for them as yet) in lucene/core/src/java/org/apache/lucene/index
            • MergePolicyFactory[Helper|Args]
            • (Simple|Wrapper)MergePolicyFactory
            • (Tiered|UpgradeIndex)MergePolicyFactory (additional factories needed e.g. for NoMergePolicy and LogMergePolicy)

          solr:

          • SolrIndexConfig.java changes to add <mergePolicyFactory> support and deprecate (but maintain) <mergePolicy> element support
          • solrconfig-tieredmergepolicy.xml added based on solrconfig-tieredmergepolicy.xml with tests randomly choosing one or the other (similar would be needed for other test solrconfig.xml files)

          other:

          • example and documentation changes - TODO
          Show
          ASF subversion and git services added a comment - Commit 3c4929e8efa041d53117e6f3669485d96839c98d in lucene-solr's branch refs/heads/master-solr-8621 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3c4929e ] [outline] SOLR-8621 : solrconfig.xml: deprecate/replace <mergePolicy> with <mergePolicyFactory> outline summary: subtleties: We currently have mergeFactor and maxMergeDocs elements alongside the mergePolicy element. The mergeFactor/maxMergeDocs/mergePolicy elements will be deprecated together i.e. mergePolicyFactory will ignore mergeFactor and maxMergeDocs. (Note that maxMergeDocs only applies to LogMergePolicy policies, and that mergeFactor only applies to LogMergePolicy and TieredMergePolicy policies.) We currently have SolrIndexConfig.fixUseCFMergePolicyInitArg to account for any <useCompoundFile> in <mergePolicy> (since ?.? <useCompoundFile> is directly specified in <indexConfig>). For the <mergePolicyFactory> element no such accommodations would be made. independent: some invokeSetter functionality from solr/util/SolrPluginUtils factored out into lucene/util/LucenePluginUtils (SolrPluginUtilsTest would need similar factoring out) lucene: new classes (but no tests for them as yet) in lucene/core/src/java/org/apache/lucene/index MergePolicyFactory [Helper|Args] (Simple|Wrapper)MergePolicyFactory (Tiered|UpgradeIndex)MergePolicyFactory (additional factories needed e.g. for NoMergePolicy and LogMergePolicy) solr: SolrIndexConfig.java changes to add <mergePolicyFactory> support and deprecate (but maintain) <mergePolicy> element support solrconfig-tieredmergepolicy.xml added based on solrconfig-tieredmergepolicy.xml with tests randomly choosing one or the other (similar would be needed for other test solrconfig.xml files) other: example and documentation changes - TODO
          Hide
          ASF subversion and git services added a comment -

          Commit 14d2b0c32d56da20ef1f8c2026f747640f7109d2 in lucene-solr's branch refs/heads/master-solr-8621 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=14d2b0c ]

          [work-in-progress] SOLR-8621: missed out .../test-files/.../solrconfig-tieredmergepolicyfactory.xml
          in initial commit

          Show
          ASF subversion and git services added a comment - Commit 14d2b0c32d56da20ef1f8c2026f747640f7109d2 in lucene-solr's branch refs/heads/master-solr-8621 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=14d2b0c ] [work-in-progress] SOLR-8621 : missed out .../test-files/.../solrconfig-tieredmergepolicyfactory.xml in initial commit
          Hide
          Jack Krupansky added a comment -

          Is this simply a rename of the XML element name (from <mergePolicy> to <mergePolicyFactory>) or is there some other user-visible feature enhancement or change?

          Is Fix Version 6.0?

          Show
          Jack Krupansky added a comment - Is this simply a rename of the XML element name (from <mergePolicy> to <mergePolicyFactory>) or is there some other user-visible feature enhancement or change? Is Fix Version 6.0?
          Hide
          Christine Poerschke added a comment -

          Is this simply a rename of the XML element name (from <mergePolicy> to <mergePolicyFactory>) or is there some other user-visible feature enhancement or change?

          Good question. This is more-or-less-but-not-quite a simple rename (see illustration below). The end-user benefit of the change is to make policies such as Lucene's UpgradeIndexMergePolicy configurable in Solr (see illustration above). The <mergePolicyFactory> element will also encompass all merge policy related parameters whereas currently <mergeFactor> and <maxMergeDocs> elements (applicable for some but not all merge policies) can be 'alongside' the <mergePolicy> element.

          existing/deprecated:

          <mergeFactor>12</mergeFactor>
          <maxMergeDocs>345</maxMergeDocs>
          <mergePolicy class="..." />
            <str name="abc">def</str>
          </mergePolicy>
          

          replacement:

          <mergePolicyFactory class="...Factory">
            <int name="mergeFactor">12</int>
            <int name="maxMergeDocs">345</int>
            <str name="abc">def</str>
          </mergePolicyFactory>
          
          Show
          Christine Poerschke added a comment - Is this simply a rename of the XML element name (from <mergePolicy> to <mergePolicyFactory>) or is there some other user-visible feature enhancement or change? Good question. This is more-or-less-but-not-quite a simple rename (see illustration below). The end-user benefit of the change is to make policies such as Lucene's UpgradeIndexMergePolicy configurable in Solr (see illustration above). The <mergePolicyFactory> element will also encompass all merge policy related parameters whereas currently <mergeFactor> and <maxMergeDocs> elements (applicable for some but not all merge policies) can be 'alongside' the <mergePolicy> element. existing/deprecated: <mergeFactor>12</mergeFactor> <maxMergeDocs>345</maxMergeDocs> <mergePolicy class= "..." /> <str name= "abc" >def</str> </mergePolicy> replacement: <mergePolicyFactory class= "...Factory" > < int name= "mergeFactor" >12</ int > < int name= "maxMergeDocs" >345</ int > <str name= "abc" >def</str> </mergePolicyFactory>
          Hide
          Christine Poerschke added a comment -

          Is Fix Version 6.0?

          The proposed roadmap (i will update the ticket summary to reflect that it is only a proposed roadmap at this stage) is that Version 5.5 introduces <mergePolicyFactory> and deprecates <mergePolicy> and that Version 6.0 removes <mergePolicy> support.

          Show
          Christine Poerschke added a comment - Is Fix Version 6.0? The proposed roadmap (i will update the ticket summary to reflect that it is only a proposed roadmap at this stage) is that Version 5.5 introduces <mergePolicyFactory> and deprecates <mergePolicy> and that Version 6.0 removes <mergePolicy> support.
          Hide
          Christine Poerschke added a comment -

          Noticed the two LUCENE-7005 things whilst working on SOLR-8621 but really the former is separate from the latter.

          Show
          Christine Poerschke added a comment - Noticed the two LUCENE-7005 things whilst working on SOLR-8621 but really the former is separate from the latter.
          Hide
          Hoss Man added a comment -

          (proposed) roadmap:

          A pattern we've followed in the past is to make those deprecations/failures contingent on luceneMatchVersion.

          So a person using Solr X >= 5.5 with a luceneMatchVersion < 5.5 gets a warning that mergePolicy is deprecated. but if they change their luceneMatchVersion >= 5.5 then it starts being an error.

          That way users who do a drop in upgrade w/o changing configs just get a handy warning, but new users who start with an example config that has luceneMatchVersion >= 5.5 will get a helpful fail-fast error if they try to follow old advice/docs and add something that's already dperecated.

          (most of the places you see assertWarnOrFail(..., true) used use to have a luceneMatchVersion comparison done in the last param, before those very old version constants (ie pre 5.0) were completely removed)

          Show
          Hoss Man added a comment - (proposed) roadmap: A pattern we've followed in the past is to make those deprecations/failures contingent on luceneMatchVersion. So a person using Solr X >= 5.5 with a luceneMatchVersion < 5.5 gets a warning that mergePolicy is deprecated. but if they change their luceneMatchVersion >= 5.5 then it starts being an error. That way users who do a drop in upgrade w/o changing configs just get a handy warning, but new users who start with an example config that has luceneMatchVersion >= 5.5 will get a helpful fail-fast error if they try to follow old advice/docs and add something that's already dperecated. (most of the places you see assertWarnOrFail(..., true) used use to have a luceneMatchVersion comparison done in the last param, before those very old version constants (ie pre 5.0) were completely removed)
          Hide
          Jack Krupansky added a comment -

          Will both the <mergeFactor> and <maxMergeDocs> elements will be deprecated as well (in addition to being allowed within the new <mergPolicyFactory>)?

          Show
          Jack Krupansky added a comment - Will both the <mergeFactor> and <maxMergeDocs> elements will be deprecated as well (in addition to being allowed within the new <mergPolicyFactory>)?
          Hide
          Jack Krupansky added a comment -

          IIUC, the motivation here is to permit any number of merge policies to be configured with the goal of supporting wrapping of merge policies. Okay, but what tells Solr which MP is the outer/default MP?

          Show
          Jack Krupansky added a comment - IIUC, the motivation here is to permit any number of merge policies to be configured with the goal of supporting wrapping of merge policies. Okay, but what tells Solr which MP is the outer/default MP?
          Hide
          Shai Erera added a comment -

          Will both the <mergeFactor> and <maxMergeDocs> elements will be deprecated as well

          Yes

          IIUC, the motivation here is to permit any number of merge policies to be configured with the goal of supporting wrapping of merge policies. Okay, but what tells Solr which MP is the outer/default MP?

          The intention is to have a MergePolicyFactory which creates a MergePolicy instance. See Christine Poerschke description above for the proposed structure in solrconfig.xml. If you want to construct your own MP, maybe it's a compound MP, you can simply define the class name of your factory, and then pass whatever arguments inside.

          Show
          Shai Erera added a comment - Will both the <mergeFactor> and <maxMergeDocs> elements will be deprecated as well Yes IIUC, the motivation here is to permit any number of merge policies to be configured with the goal of supporting wrapping of merge policies. Okay, but what tells Solr which MP is the outer/default MP? The intention is to have a MergePolicyFactory which creates a MergePolicy instance. See Christine Poerschke description above for the proposed structure in solrconfig.xml . If you want to construct your own MP, maybe it's a compound MP, you can simply define the class name of your factory, and then pass whatever arguments inside.
          Hide
          Shai Erera added a comment -

          Christine Poerschke about the commit, I have few comments:

          • MergePolicyFactory, IMO, should belong to Solr. I don't see it being used by Lucene users, as they already construct IndexWriterConfig which takes a MergePolicy instance. The factory, again IMO, is useful in Solr-land, because Solr is configured from a file and therefore being able to construct an MP from a set of properties will be useful to Solr users.
          • I don't fully get the purpose of MergePolicyFactoryHelper. Did you create that abstraction so we can use it in Solr? If we move MPF to Solr, I think we can get rid of it? Its only purpose as I can tell is to create an instance of an MP, but I thought that's what the factory will do?
          • SimpleMergePolicyFactory is what I had in mind for the simple MPs. I wonder if we took the actual MPs class name in the properties, we wouldn't need TieredMPF, and will also simply support LogMP. What do you think? The XML element would look like:
          <mergePolicyFactory class="solr.SimpleMergePolicyFactory">
            <str name="class">solr.TieredMergePolicy</str>
            ...
          </mergePolicyFactory>
          

          What do you think?

          • Is it OK if I make some changes to the code and push to the branch, or are you actively working on it?
          Show
          Shai Erera added a comment - Christine Poerschke about the commit, I have few comments: MergePolicyFactory , IMO, should belong to Solr. I don't see it being used by Lucene users, as they already construct IndexWriterConfig which takes a MergePolicy instance. The factory, again IMO, is useful in Solr-land, because Solr is configured from a file and therefore being able to construct an MP from a set of properties will be useful to Solr users. I don't fully get the purpose of MergePolicyFactoryHelper . Did you create that abstraction so we can use it in Solr? If we move MPF to Solr, I think we can get rid of it? Its only purpose as I can tell is to create an instance of an MP, but I thought that's what the factory will do? SimpleMergePolicyFactory is what I had in mind for the simple MPs. I wonder if we took the actual MPs class name in the properties, we wouldn't need TieredMPF , and will also simply support LogMP . What do you think? The XML element would look like: <mergePolicyFactory class= "solr.SimpleMergePolicyFactory" > <str name= "class" >solr.TieredMergePolicy</str> ... </mergePolicyFactory> What do you think? Is it OK if I make some changes to the code and push to the branch, or are you actively working on it?
          Hide
          Christine Poerschke added a comment -

          Shai Erera - thanks for your comments.

          • Yes, the purpose of MergePolicyFactoryHelper was to allow the lucene factory to use the method currently used by solr to create new instances but if the factories belonged to Solr instead of Lucene then no helper would be needed.
          • SimpleMergePolicyFactory with a class property - yes, that sounds good to me.
          • master-solr-8621 just updated to merge in LUCENE-7005 and LUCENE-7006 from master. I've got one more push to the branch in the next hour or so but then don't expect to be working on it until Thursday at the earliest, though hoping take a look at the LUCENE-7006 TestSortingMergePolicy test failure at some point.
          Show
          Christine Poerschke added a comment - Shai Erera - thanks for your comments. Yes, the purpose of MergePolicyFactoryHelper was to allow the lucene factory to use the method currently used by solr to create new instances but if the factories belonged to Solr instead of Lucene then no helper would be needed. SimpleMergePolicyFactory with a class property - yes, that sounds good to me. master-solr-8621 just updated to merge in LUCENE-7005 and LUCENE-7006 from master. I've got one more push to the branch in the next hour or so but then don't expect to be working on it until Thursday at the earliest, though hoping take a look at the LUCENE-7006 TestSortingMergePolicy test failure at some point.
          Hide
          ASF subversion and git services added a comment -

          Commit 43671b3e0dc998bab73fd53bcd4df829d8eb273d in lucene-solr's branch refs/heads/master-solr-8621 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=43671b3 ]

          for SOLR-8621 work-in-progress branch:

          • adds LogByteSizeMergePolicyFactory and LogDocMergePolicyFactory
          • removes subPackages arg from MergePolicyFactoryHelper.newInstance signature
          • adds BaseMergePolicyTestCase.testMergePolicyFactory method (and the things it calls)
          Show
          ASF subversion and git services added a comment - Commit 43671b3e0dc998bab73fd53bcd4df829d8eb273d in lucene-solr's branch refs/heads/master-solr-8621 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=43671b3 ] for SOLR-8621 work-in-progress branch: adds LogByteSizeMergePolicyFactory and LogDocMergePolicyFactory removes subPackages arg from MergePolicyFactoryHelper.newInstance signature adds BaseMergePolicyTestCase.testMergePolicyFactory method (and the things it calls)
          Hide
          Shai Erera added a comment -

          Thanks Christine Poerschke. I've also started to make some changes, so I'll rebase on yours after you push them. If you haven't done so already, I'll move the factory under o.a.solr and adjust the relevant classes.

          BTW, I think we should split this into multiple commits:

          1. Introduce MergePolicyFactory and some implementing classes. Add a warning to SolrIndexConfig. That way, all existing solrconfig.xml will continue to work.
          2. Change all solrconfig.xml to use the new <mergePolicyFactory>, and change the warning to an error in SolrIndexConfig on trunk and keep the warning on 5x.

          Also, I would personally start with only supporting simple MPF for now, because the wrapped ones need a bit of discussion I feel (per the review I've done). E.g. UpgradeIndexMergePolicyFactory hardcodes the prefix properties to "base", but it's not documented anywhere. I think that we can generalize the whole idea of wrapping MPF with something like this (indentations for clarity only)

          <!-- public MyInstrumentedMergePolicy(MergePolicy mergePolicy) { -->
          <mergePolicyFactory class="MyInstrumentedMergePolicyFactory">
            <int name="instrumentationLevel">42</int>
            <str name="wrapped.key">mergePolicy</str>
              <str name="mergePolicy">SortingMergePolicyFactory</str>
              <str name="mergePolicy.sort">timestamp desc</str>
              <str name="mergePolicy.wrapped.key">in</str>
                <str name="mergePolicy.in">TieredMergePolicyFactory</str>
                <int name="mergePolicy.in.segmentsPerTier">42</int>
          </mergePolicyFactory>
          

          Also, I feel that having a fixed/expected key name for the class will be useful in the code too, e.g.:

          <!-- public MyInstrumentedMergePolicy(MergePolicy mergePolicy) { -->
          <mergePolicyFactory class="MyInstrumentedMergePolicyFactory">
            <int name="instrumentationLevel">42</int>
            <str name="wrapped.key">mergePolicy</str>
              <str name="mergePolicy.class">SortingMergePolicyFactory</str>
              <str name="mergePolicy.sort">timestamp desc</str>
              <str name="mergePolicy.wrapped.key">in</str>
                <str name="mergePolicy.in.class">TieredMergePolicyFactory</str>
                <int name="mergePolicy.in.segmentsPerTier">42</int>
          </mergePolicyFactory>
          

          It's then both consistent with the wrapped <mergePolicyFactory> element (taking a 'class') and also in the code we can just do args.get("class") and fail early if none is defined. What do you think?

          Show
          Shai Erera added a comment - Thanks Christine Poerschke . I've also started to make some changes, so I'll rebase on yours after you push them. If you haven't done so already, I'll move the factory under o.a.solr and adjust the relevant classes. BTW, I think we should split this into multiple commits: Introduce MergePolicyFactory and some implementing classes. Add a warning to SolrIndexConfig . That way, all existing solrconfig.xml will continue to work. Change all solrconfig.xml to use the new <mergePolicyFactory> , and change the warning to an error in SolrIndexConfig on trunk and keep the warning on 5x. Also, I would personally start with only supporting simple MPF for now, because the wrapped ones need a bit of discussion I feel (per the review I've done). E.g. UpgradeIndexMergePolicyFactory hardcodes the prefix properties to "base", but it's not documented anywhere. I think that we can generalize the whole idea of wrapping MPF with something like this (indentations for clarity only) <!-- public MyInstrumentedMergePolicy(MergePolicy mergePolicy) { --> <mergePolicyFactory class= "MyInstrumentedMergePolicyFactory" > < int name= "instrumentationLevel" >42</ int > <str name= "wrapped.key" >mergePolicy</str> <str name= "mergePolicy" >SortingMergePolicyFactory</str> <str name= "mergePolicy.sort" >timestamp desc</str> <str name= "mergePolicy.wrapped.key" >in</str> <str name= "mergePolicy.in" >TieredMergePolicyFactory</str> < int name= "mergePolicy.in.segmentsPerTier" >42</ int > </mergePolicyFactory> Also, I feel that having a fixed/expected key name for the class will be useful in the code too, e.g.: <!-- public MyInstrumentedMergePolicy(MergePolicy mergePolicy) { --> <mergePolicyFactory class= "MyInstrumentedMergePolicyFactory" > < int name= "instrumentationLevel" >42</ int > <str name= "wrapped.key" >mergePolicy</str> <str name= "mergePolicy.class" >SortingMergePolicyFactory</str> <str name= "mergePolicy.sort" >timestamp desc</str> <str name= "mergePolicy.wrapped.key" >in</str> <str name= "mergePolicy.in.class" >TieredMergePolicyFactory</str> < int name= "mergePolicy.in.segmentsPerTier" >42</ int > </mergePolicyFactory> It's then both consistent with the wrapped <mergePolicyFactory> element (taking a 'class') and also in the code we can just do args.get("class") and fail early if none is defined. What do you think?
          Hide
          Christine Poerschke added a comment -

          BTW, I think we should split this into multiple commits: ...

          I am a big fan of multiple commits if that makes logically sense and can see it applying to this ticket also.

          ... start with only supporting simple MPF for now ...

          Sounds good. Though both simple MPF and wrapped MPF support should from the end-user perspective be in the same solr release in my opinion since otherwise there will be this new <mergePolicyFactory> element in solrconfig.xml without an immediate end-user benefit.

          ... <str name="mergePolicy.wrapped.key">in</str> ...

          Excellent idea, I like it. Need we consider the possibility of a custom merge policy factory wrapping more-than-one merge policy?

          PS: I have pushed all my current changes to master-solr-8621 as per above. Thanks!

          Show
          Christine Poerschke added a comment - BTW, I think we should split this into multiple commits: ... I am a big fan of multiple commits if that makes logically sense and can see it applying to this ticket also. ... start with only supporting simple MPF for now ... Sounds good. Though both simple MPF and wrapped MPF support should from the end-user perspective be in the same solr release in my opinion since otherwise there will be this new <mergePolicyFactory> element in solrconfig.xml without an immediate end-user benefit. ... <str name="mergePolicy.wrapped.key">in</str> ... Excellent idea, I like it. Need we consider the possibility of a custom merge policy factory wrapping more-than-one merge policy? PS: I have pushed all my current changes to master-solr-8621 as per above. Thanks!
          Hide
          Shai Erera added a comment -

          Though both simple MPF and wrapped MPF support should from the end-user perspective be in the same solr release

          Well, Simple for sure because that's what users can do today. No one, as far as I can tell, define today a compound MP like SortingMP or UpgradeIndexMP without defining his own MP class which will delegate to them. So introduce MergePolicyFactory which can allow you to conveniently do that in solrconfig.xml is a new feature. But, we want to get them both out in 5.5, so no need to argue about it, let's just make it happen. If 5.5 was a week away, we'd perhaps want to finish working on Simple first, but it's not.

          Need we consider the possibility of a custom merge policy factory wrapping more-than-one merge policy?

          Do you mean an MP wrapping an MP wrapping an MP? I am not aware of any MP in Lucene today that behaves like that, but I think a WrapperMPF should just seamlessly allow it?

          PS: I have pushed all my current changes to master-solr-8621 as per above. Thanks!

          Thanks and I've already rebased. I'll move the classes under *.solr now and get the next set of changes in reasonable shape and push them. From a multiple commits standpoint, we can do the following:

          • Finish current work stream by introducing Simple/Wrapper etc. Once we're satisfied with these changes (i.e. willing to commit to trunk/5x) we squash the commits into one and push to master/branch_5x.
          • Handle the warning/error path, by modifying solrconfig.xmls, failing in trunk and warning in 5x. Push those to master/branch_5x.
          • Handle SortingMergePolicy on the other issue.

          What do you think?

          Show
          Shai Erera added a comment - Though both simple MPF and wrapped MPF support should from the end-user perspective be in the same solr release Well, Simple for sure because that's what users can do today. No one, as far as I can tell, define today a compound MP like SortingMP or UpgradeIndexMP without defining his own MP class which will delegate to them. So introduce MergePolicyFactory which can allow you to conveniently do that in solrconfig.xml is a new feature. But, we want to get them both out in 5.5, so no need to argue about it, let's just make it happen. If 5.5 was a week away, we'd perhaps want to finish working on Simple first, but it's not. Need we consider the possibility of a custom merge policy factory wrapping more-than-one merge policy? Do you mean an MP wrapping an MP wrapping an MP? I am not aware of any MP in Lucene today that behaves like that, but I think a WrapperMPF should just seamlessly allow it? PS: I have pushed all my current changes to master-solr-8621 as per above. Thanks! Thanks and I've already rebased. I'll move the classes under *.solr now and get the next set of changes in reasonable shape and push them. From a multiple commits standpoint, we can do the following: Finish current work stream by introducing Simple/Wrapper etc. Once we're satisfied with these changes (i.e. willing to commit to trunk/5x) we squash the commits into one and push to master/branch_5x. Handle the warning/error path, by modifying solrconfig.xmls, failing in trunk and warning in 5x. Push those to master/branch_5x. Handle SortingMergePolicy on the other issue. What do you think?
          Hide
          Christine Poerschke added a comment -

          Need we consider the possibility of a custom merge policy factory wrapping more-than-one merge policy?

          Illustration (using admittedly contrived hypothetical examples):

          public SeasonalMergePolicy(MergePolicy spring, MergePolicy summer, MergePolicy autumn, MergePolicy winter) {
          public WorkingWeekMergePolicy(MergePolicy weekday, MergePolicy weekend) {
          public CloudAwareMergePolicy(MergePolicy busyCloud, MergePolicy unbusyCloud) {
          
          Show
          Christine Poerschke added a comment - Need we consider the possibility of a custom merge policy factory wrapping more-than-one merge policy? Illustration (using admittedly contrived hypothetical examples): public SeasonalMergePolicy(MergePolicy spring, MergePolicy summer, MergePolicy autumn, MergePolicy winter) { public WorkingWeekMergePolicy(MergePolicy weekday, MergePolicy weekend) { public CloudAwareMergePolicy(MergePolicy busyCloud, MergePolicy unbusyCloud) {
          Hide
          Shai Erera added a comment -

          Hmm .. interesting ideas . In that case I think we can change "wrapped.key" to take a comma-separated list of keys and then create an appropriate factory class? Or, we don't change anything and just create an appropriate MPF for each of them, taking whatever parameters we need? I'd do that though in a separate issue.

          Show
          Shai Erera added a comment - Hmm .. interesting ideas . In that case I think we can change "wrapped.key" to take a comma-separated list of keys and then create an appropriate factory class? Or, we don't change anything and just create an appropriate MPF for each of them, taking whatever parameters we need? I'd do that though in a separate issue.
          Hide
          Christine Poerschke added a comment -

          I'd do that though in a separate issue.

          Yep, totally that can be a separate issue and for now we just need to keep in mind that a merge policy that wraps two or more other merge policies could exists at some point in the future.

          Show
          Christine Poerschke added a comment - I'd do that though in a separate issue. Yep, totally that can be a separate issue and for now we just need to keep in mind that a merge policy that wraps two or more other merge policies could exists at some point in the future.
          Hide
          Shai Erera added a comment -

          Christine Poerschke I've pushed my changes to the branch. You're welcome to review it.

          Show
          Shai Erera added a comment - Christine Poerschke I've pushed my changes to the branch. You're welcome to review it.
          Hide
          Shai Erera added a comment -

          Christine Poerschke I've one question about the branch you created. It seemed that when you updated it to latest master, you did that with a git merge command - correct? So if now I type 'git log' I see something like this:

          solr-8621 commit
          solr-8621 commit
          non-solr-8621 commit
          non-solr-8621 commit
          non-solr-8621 commit
          non-solr-8621 commit
          non-solr-8621 commit
          non-solr-8621 commit
          non-solr-8621 commit
          solr-8621 commit
          solr-8621 commit
          

          I know we're still discussing rebase vs merge on that thread, and I don't mean to continue the discussion here. I just want to level set with your expectations and understand if you did that intentionally, i.e. is that how you expect 'git log' to look eventually, or was that done as an afterthought?

          I would personally rebase all commits we do in this branch on master, so that eventually we can have the commits stacked on top of the latest one in master. I would even squash all of them into a single commit which says "SOLR-8621: add MergePolicyFactory" and with a broader description.

          So again, just mean to level set with your intentions here .

          Show
          Shai Erera added a comment - Christine Poerschke I've one question about the branch you created. It seemed that when you updated it to latest master, you did that with a git merge command - correct? So if now I type 'git log' I see something like this: solr-8621 commit solr-8621 commit non-solr-8621 commit non-solr-8621 commit non-solr-8621 commit non-solr-8621 commit non-solr-8621 commit non-solr-8621 commit non-solr-8621 commit solr-8621 commit solr-8621 commit I know we're still discussing rebase vs merge on that thread, and I don't mean to continue the discussion here. I just want to level set with your expectations and understand if you did that intentionally, i.e. is that how you expect 'git log' to look eventually, or was that done as an afterthought? I would personally rebase all commits we do in this branch on master, so that eventually we can have the commits stacked on top of the latest one in master. I would even squash all of them into a single commit which says " SOLR-8621 : add MergePolicyFactory" and with a broader description. So again, just mean to level set with your intentions here .
          Hide
          Christine Poerschke added a comment -

          I've pushed my changes to the branch. You're welcome to review it.

          Great, thanks! Will take a look, very likely not before Thursday though.

          I've one question about the branch you created. ...

          Correct, I used git merge master to merge the latest (at the time) changes from master onto master-solr-8621 branch. (This unexpectedly added updates to the tickets concerned which might be confusing, but that's a separate discussion for elsewhere.) My intention was for the master-solr-8621 history to reflect its detailed history. As far as 'committing to master' is concerned (at least with respect to this SOLR-8621 ticket here) my outlook currently is that all the master-solr-8621 history would be squashed 'at the end' and going back to 'master' there would be just a single commit. How does that sound (with respect to this ticket, not generally speaking)?

          Show
          Christine Poerschke added a comment - I've pushed my changes to the branch. You're welcome to review it. Great, thanks! Will take a look, very likely not before Thursday though. I've one question about the branch you created. ... Correct, I used git merge master to merge the latest (at the time) changes from master onto master-solr-8621 branch. (This unexpectedly added updates to the tickets concerned which might be confusing, but that's a separate discussion for elsewhere.) My intention was for the master-solr-8621 history to reflect its detailed history. As far as 'committing to master' is concerned (at least with respect to this SOLR-8621 ticket here) my outlook currently is that all the master-solr-8621 history would be squashed 'at the end' and going back to 'master' there would be just a single commit. How does that sound (with respect to this ticket, not generally speaking)?
          Hide
          Shai Erera added a comment -

          my outlook currently is that all the master-solr-8621 history would be squashed 'at the end' and going back to 'master' there would be just a single commit. How does that sound

          That sounds just like I would prefer it to happen, so glad to see we're in agreement here. Just FYI, it would have been easier to do this squash if you rebased master-solr-8621 on origin/master. Then all our commits would stack on top of each other.

          We can still do this though, but I think we'd need to cherry-pick all commits there into a separate local branch, and then squash. Not sure it matters much now though. I don't mind doing the final "commit" after we finish iterating on the changes.

          Show
          Shai Erera added a comment - my outlook currently is that all the master-solr-8621 history would be squashed 'at the end' and going back to 'master' there would be just a single commit. How does that sound That sounds just like I would prefer it to happen, so glad to see we're in agreement here. Just FYI, it would have been easier to do this squash if you rebased master-solr-8621 on origin/master. Then all our commits would stack on top of each other. We can still do this though, but I think we'd need to cherry-pick all commits there into a separate local branch, and then squash. Not sure it matters much now though. I don't mind doing the final "commit" after we finish iterating on the changes.
          Hide
          Shai Erera added a comment -

          Christine Poerschke have you had a chance to review the latest changes yet?

          Show
          Shai Erera added a comment - Christine Poerschke have you had a chance to review the latest changes yet?
          Hide
          Christine Poerschke added a comment -

          Christine Poerschke have you had a chance to review the latest changes yet?

          Yes, literally having a look at now/this evening, looks good Am tinkering a little with tests and exploring if a DefaultMergePolicyFactory could make an appearance (as-is both SolrIndexConfig and UpgradeIndexMergePolicy have the concept of TieredMergePolicy being the default). Hoping to push update to master-solr-8621 branch in the next hour or so.

          Show
          Christine Poerschke added a comment - Christine Poerschke have you had a chance to review the latest changes yet? Yes, literally having a look at now/this evening, looks good Am tinkering a little with tests and exploring if a DefaultMergePolicyFactory could make an appearance (as-is both SolrIndexConfig and UpgradeIndexMergePolicy have the concept of TieredMergePolicy being the default). Hoping to push update to master-solr-8621 branch in the next hour or so.
          Hide
          Christine Poerschke added a comment -

          Christine Poerschke have you had a chance to review the latest changes yet?

          Pushed my changes to master-solr-8621 branch. Shai Erera - let me know what you think.

          For WrapperMergePolicyFactoryTest I'd like to add one more test, tomorrow hopefully, it currently tests that the wrapped policy correctly 'claims' its arguments from the args list and it would be good to also test that the wrapped policy is not wrongly claiming arguments belonging to either the wrapping policy nor arguments belonging to another wrapped policy.

          ... I don't mind doing the final "commit" after we finish iterating on the changes.

          Sure, that works for me.

          Show
          Christine Poerschke added a comment - Christine Poerschke have you had a chance to review the latest changes yet? Pushed my changes to master-solr-8621 branch. Shai Erera - let me know what you think. For WrapperMergePolicyFactoryTest I'd like to add one more test, tomorrow hopefully, it currently tests that the wrapped policy correctly 'claims' its arguments from the args list and it would be good to also test that the wrapped policy is not wrongly claiming arguments belonging to either the wrapping policy nor arguments belonging to another wrapped policy. ... I don't mind doing the final "commit" after we finish iterating on the changes. Sure, that works for me.
          Hide
          Shai Erera added a comment -

          Thanks Christine Poerschke, I'll review the two commits that you pushed. One quick comment/question that I have is about SolrIndexConfig's defaultMP class and factory names. (1) I think they can be private since they're used only in SolrIndexConfig. Two, I think we can use an instance, rather than class name? So that in buildMPFromInfo we just create a new instance (i.e. new DefaultMPFactory().getMP()). What do you think?

          I'll give it another review later.

          Show
          Shai Erera added a comment - Thanks Christine Poerschke , I'll review the two commits that you pushed. One quick comment/question that I have is about SolrIndexConfig 's defaultMP class and factory names. (1) I think they can be private since they're used only in SolrIndexConfig . Two, I think we can use an instance, rather than class name? So that in buildMPFromInfo we just create a new instance (i.e. new DefaultMPFactory().getMP() ). What do you think? I'll give it another review later.
          Hide
          Christine Poerschke added a comment -

          Hello Shai Erera, thanks for taking a look.

          ... I think they can be private since they're used only in SolrIndexConfig

          I had considered package-visibility but assumed/think it's not possible since SolrIndexConfig is org.apache.solr.update and DefaultMergePolicyFactory is org.apache.solr.index. But your second point would make this moot anyhow.

          ... in buildMPFromInfo we just create a new instance (i.e. new DefaultMPFactory().getMP()).

          Yes, that totally makes sense. My focus had been on having a single unified place where 'default merge policy-ness' is handled, changing the existing buildMergePolicy[FromInfo] code to also use the new default factory simply hadn't appeared on the radar. Good find! I'll go make that change.

          Unrelated to the above, just pushed changes to MergePolicyFactoryArgs which will do away with the need to change SolrPluginUtils but still keep MergePolicyFactoryArgs as an abstraction which just happens to be implemented via a NamedList at the moment.

          Show
          Christine Poerschke added a comment - Hello Shai Erera , thanks for taking a look. ... I think they can be private since they're used only in SolrIndexConfig I had considered package-visibility but assumed/think it's not possible since SolrIndexConfig is org.apache.solr.update and DefaultMergePolicyFactory is org.apache.solr.index. But your second point would make this moot anyhow. ... in buildMPFromInfo we just create a new instance (i.e. new DefaultMPFactory().getMP()). Yes, that totally makes sense. My focus had been on having a single unified place where 'default merge policy-ness' is handled, changing the existing buildMergePolicy[FromInfo] code to also use the new default factory simply hadn't appeared on the radar. Good find! I'll go make that change. Unrelated to the above, just pushed changes to MergePolicyFactoryArgs which will do away with the need to change SolrPluginUtils but still keep MergePolicyFactoryArgs as an abstraction which just happens to be implemented via a NamedList at the moment.
          Hide
          Christine Poerschke added a comment -

          One more thing: release 5.5 branch cutting is on the horizon. Are we intending to be in 5.5 with SOLR-8621? I think it would be good to be in 5.5 release. What do you think?

          Show
          Christine Poerschke added a comment - One more thing: release 5.5 branch cutting is on the horizon. Are we intending to be in 5.5 with SOLR-8621 ? I think it would be good to be in 5.5 release. What do you think?
          Hide
          Shai Erera added a comment -

          Christine Poerschke, IMO we should definitely aim at releasing it in 5.5, since we have to deprecate the <mergePolicy> stuff in solrconfig.xml. So let's mark this issue as a Blocker for 5.5. I suspect 5.5 will take a while, especially if it's supposed to be the last 5.x release before 6.0, cause a lot of stuff will need to be properly deprecated. Anyway, I think we're close and can probably finish this one by next week.

          Show
          Shai Erera added a comment - Christine Poerschke , IMO we should definitely aim at releasing it in 5.5, since we have to deprecate the <mergePolicy> stuff in solrconfig.xml . So let's mark this issue as a Blocker for 5.5. I suspect 5.5 will take a while, especially if it's supposed to be the last 5.x release before 6.0, cause a lot of stuff will need to be properly deprecated. Anyway, I think we're close and can probably finish this one by next week.
          Hide
          Christine Poerschke added a comment -

          Shai Erera - my last push to master-solr-8621 proposes to make SimpleMergePolicyFactory non-abstract and considers (in the admittedly rather long commit message)

          • whether or not (Log(ByteSize|Doc)|Tiered)MergePolicyFactory should be retained, and
          • whether or not WrapperMergePolicyFactory should support a "class" element.

          I'm thinking "no" and "yes" - what do you think?

          Show
          Christine Poerschke added a comment - Shai Erera - my last push to master-solr-8621 proposes to make SimpleMergePolicyFactory non-abstract and considers (in the admittedly rather long commit message) whether or not (Log(ByteSize|Doc)|Tiered)MergePolicyFactory should be retained, and whether or not WrapperMergePolicyFactory should support a "class" element. I'm thinking "no" and "yes" - what do you think?
          Hide
          Shai Erera added a comment -

          Christine Poerschke I reviewed the latest patch and here's my take on SimpleMergePolicyFactory: at first, I thought about this approach too, but eventually preferred the explicit TieredMPFactory. It has few advantages IMO:

          • It's simple. Requires less typing by the user, and having a SimpleMPF for which you declare a 'class' feels like a level of indirection. It's as if SimpleMPF itself has no use.
          • WrapperMPF is also a code-only class, but users don't specify it explicitly in config.xml. Users would specify a SortingMPF or UpgradeIndexMPF.
          • The 'class' element of WrapperMPF is a Factory class, not an MP class. Unlike the one we'll have on SimpleMPF.

          If we'll have both SimpleMPF and TieredMPF, it feels like duplicated efforts. Perhaps we should start simple (i.e. keep SimpleMPF abstract and only expose the concrete factories) and if demand arises, we can make it a concrete class too? That route is always back-compat, but the other one may not be.

          So Simple/Wrapper belong more to the code-design land, while the concrete factories belong to the user land.

          What do you think?

          Show
          Shai Erera added a comment - Christine Poerschke I reviewed the latest patch and here's my take on SimpleMergePolicyFactory : at first, I thought about this approach too, but eventually preferred the explicit TieredMPFactory . It has few advantages IMO: It's simple. Requires less typing by the user, and having a SimpleMPF for which you declare a 'class' feels like a level of indirection. It's as if SimpleMPF itself has no use. WrapperMPF is also a code-only class, but users don't specify it explicitly in config.xml. Users would specify a SortingMPF or UpgradeIndexMPF. The 'class' element of WrapperMPF is a Factory class, not an MP class. Unlike the one we'll have on SimpleMPF. If we'll have both SimpleMPF and TieredMPF, it feels like duplicated efforts. Perhaps we should start simple (i.e. keep SimpleMPF abstract and only expose the concrete factories) and if demand arises, we can make it a concrete class too? That route is always back-compat, but the other one may not be. So Simple/Wrapper belong more to the code-design land, while the concrete factories belong to the user land. What do you think?
          Hide
          Shai Erera added a comment -

          About WrapperMergePolicyFactory, for it too I prefer that we keep the top-level concrete/exposed, and leave Wrapper to be an implementation helper.

          BTW, I thought that we can have wrapped.key optional and default to 'delegate' if one isn't specified. Again, to simplify configuration for users, who in most cases will wrap a simple MP factory. Opinions?

          In terms of 5.5, I personally feel like what we have so far is very solid and already covers all the MP scenarios I can think about. The factory itself lets you implement one from scratch and configure it however you want. And we offer helper classes (Wrapper and Simple) if you want to implement something on your own, but not from scratch. And we cover all currently supported-by-Solr MPs.

          So I'd rather we finish the work on this issue, so that 5.5 can really go out with it. We still have a lot to do in terms of actually deprecating <mergePolicy> support, remove in trunk, modify existing {{solrconfig.xml}}s etc. Then we want to expose SortingMPF (SOLR-5730), though that can happen post 5.5 if we don't make it.

          As I said in my previous comment, if we later discover that we want to make Simple/Wrapper more concrete classes, we can always do that. Post 6.0 too.

          Show
          Shai Erera added a comment - About WrapperMergePolicyFactory , for it too I prefer that we keep the top-level concrete/exposed, and leave Wrapper to be an implementation helper. BTW, I thought that we can have wrapped.key optional and default to 'delegate' if one isn't specified. Again, to simplify configuration for users, who in most cases will wrap a simple MP factory. Opinions? In terms of 5.5, I personally feel like what we have so far is very solid and already covers all the MP scenarios I can think about. The factory itself lets you implement one from scratch and configure it however you want. And we offer helper classes (Wrapper and Simple) if you want to implement something on your own, but not from scratch. And we cover all currently supported-by-Solr MPs. So I'd rather we finish the work on this issue, so that 5.5 can really go out with it. We still have a lot to do in terms of actually deprecating <mergePolicy> support, remove in trunk, modify existing {{solrconfig.xml}}s etc. Then we want to expose SortingMPF ( SOLR-5730 ), though that can happen post 5.5 if we don't make it. As I said in my previous comment, if we later discover that we want to make Simple/Wrapper more concrete classes, we can always do that. Post 6.0 too.
          Hide
          Shai Erera added a comment -

          Unrelated to the above, just pushed changes to MergePolicyFactoryArgs which will do away with the need to change SolrPluginUtils but still keep MergePolicyFactoryArgs as an abstraction which just happens to be implemented via a NamedList at the moment.

          I actually prefer that we not extend the usage of NamedList . Really what we need is a Map<String,Object> (which NamedList is also), so why introduce it in this class? Would you mind if we switch back to Map? I also think that some of the changes to SolrPluginUtils, e.g. the refactoring of the findMethod method, contributed to its code readability, so I'd like to restore them too. And finally, we we move back to a Map, then keys() don't need to create a new HashSet.

          BTW, I thought that we can have wrapped.key optional and default to 'delegate' if one isn't specified.

          I take it back for now. Currently, if wrapper.key is not specified, we assume a default wrapped MP should be used. We can still make wrapper.key optional, but I don't mind if we defer that change for now. It can always be made optional later.

          Show
          Shai Erera added a comment - Unrelated to the above, just pushed changes to MergePolicyFactoryArgs which will do away with the need to change SolrPluginUtils but still keep MergePolicyFactoryArgs as an abstraction which just happens to be implemented via a NamedList at the moment. I actually prefer that we not extend the usage of NamedList . Really what we need is a Map<String,Object> (which NamedList is also), so why introduce it in this class? Would you mind if we switch back to Map? I also think that some of the changes to SolrPluginUtils , e.g. the refactoring of the findMethod method, contributed to its code readability, so I'd like to restore them too. And finally, we we move back to a Map, then keys() don't need to create a new HashSet . BTW, I thought that we can have wrapped.key optional and default to 'delegate' if one isn't specified. I take it back for now. Currently, if wrapper.key is not specified, we assume a default wrapped MP should be used. We can still make wrapper.key optional, but I don't mind if we defer that change for now. It can always be made optional later.
          Hide
          Christine Poerschke added a comment -

          Hello Shai Erera - thanks for your comments. I'm just catching up on things here.

          ... I actually prefer that we not extend the usage of NamedList . Really what we need is a Map<String,Object> ...

          Sure, we can switch back to Map. Shall I just revert the particular commit or do we perhaps want to do away with MergePolicyFactoryArgs at this point?

          ... have wrapped.key optional and default to 'delegate' ...

          Yes, at first glance defaulting to 'delegate' sounds convenient but on second thoughts like you say wrapper.key not specified corresponding to default wrapped MP is clearer and more user friendly.

          Show
          Christine Poerschke added a comment - Hello Shai Erera - thanks for your comments. I'm just catching up on things here. ... I actually prefer that we not extend the usage of NamedList . Really what we need is a Map<String,Object> ... Sure, we can switch back to Map. Shall I just revert the particular commit or do we perhaps want to do away with MergePolicyFactoryArgs at this point? ... have wrapped.key optional and default to 'delegate' ... Yes, at first glance defaulting to 'delegate' sounds convenient but on second thoughts like you say wrapper.key not specified corresponding to default wrapped MP is clearer and more user friendly.
          Hide
          Christine Poerschke added a comment -

          re: SimpleMergePolicyFactory - I will revert the commit that made this class non-abstract. How about factoryClass (or even factory) instead of class element though? E.g.

          <mergePolicyFactory class="UpgradeMergePolicyFactory">
            <str name="wrapped.key">mergePolicy</str>
            <str name="mergePolicy.factoryClass">TieredMergePolicyFactory</str>
            <int name="mergePolicy.segmentsPerTier">42</int>
          </mergePolicyFactory>
          

          instead of

          <mergePolicyFactory class="UpgradeMergePolicyFactory">
            <str name="wrapped.key">mergePolicy</str>
            <str name="mergePolicy.class">TieredMergePolicyFactory</str>
            <int name="mergePolicy.segmentsPerTier">42</int>
          </mergePolicyFactory>
          
          Show
          Christine Poerschke added a comment - re: SimpleMergePolicyFactory - I will revert the commit that made this class non-abstract. How about factoryClass (or even factory ) instead of class element though? E.g. <mergePolicyFactory class= "UpgradeMergePolicyFactory" > <str name= "wrapped.key" >mergePolicy</str> <str name= "mergePolicy.factoryClass" >TieredMergePolicyFactory</str> < int name= "mergePolicy.segmentsPerTier" >42</ int > </mergePolicyFactory> instead of <mergePolicyFactory class= "UpgradeMergePolicyFactory" > <str name= "wrapped.key" >mergePolicy</str> <str name= "mergePolicy.class" >TieredMergePolicyFactory</str> < int name= "mergePolicy.segmentsPerTier" >42</ int > </mergePolicyFactory>
          Hide
          Shai Erera added a comment -

          Shall I just revert the particular commit or do we perhaps want to do away with MergePolicyFactoryArgs at this point?

          No need to, I'll handle that.

          I will revert the commit that made this class non-abstract

          No need to, we can simply skip it when we cherry-pick the commits.

          How about factoryClass (or even factory) instead of class element though?

          I think 'class' is more consistent with the wrapper <mergePolicyFactory class=""> element?

          Show
          Shai Erera added a comment - Shall I just revert the particular commit or do we perhaps want to do away with MergePolicyFactoryArgs at this point? No need to, I'll handle that. I will revert the commit that made this class non-abstract No need to, we can simply skip it when we cherry-pick the commits. How about factoryClass (or even factory) instead of class element though? I think 'class' is more consistent with the wrapper <mergePolicyFactory class=""> element?
          Hide
          Christine Poerschke added a comment -

          Thanks for quick reply. The piece I intend to tackle next is the solr/core/src/test-files/solr/collection1/conf solrconfig*.xml changes - is that okay? For solrconfig-tieredmergepolicy.xml there is solrconfig-tieredmergepolicyfactory.xml already and the test randomly chooses one or the other. For the other files a similar pattern would be followed and when "<mergePolicy>" support goes away the solrconfig-tieredmergepolicy.xml file goes away along with it.

          Show
          Christine Poerschke added a comment - Thanks for quick reply. The piece I intend to tackle next is the solr/core/src/test-files/solr/collection1/conf solrconfig*.xml changes - is that okay? For solrconfig-tieredmergepolicy.xml there is solrconfig-tieredmergepolicyfactory.xml already and the test randomly chooses one or the other. For the other files a similar pattern would be followed and when "<mergePolicy>" support goes away the solrconfig-tieredmergepolicy.xml file goes away along with it.
          Hide
          Shai Erera added a comment -

          We definitely should fix all the solrconfig.xmls, but I thought this can be done in a separate commit, together with decprecating <mergePolicy>. What do you think?

          For this current 'commit' I want to finish the SolrPluginUtils changes and then I thought to combine all changes we have into one commit. What do you think?

          Show
          Shai Erera added a comment - We definitely should fix all the solrconfig.xmls, but I thought this can be done in a separate commit, together with decprecating <mergePolicy> . What do you think? For this current 'commit' I want to finish the SolrPluginUtils changes and then I thought to combine all changes we have into one commit. What do you think?
          Hide
          Shai Erera added a comment -

          Christine Poerschke, patch includes the changes from all commits that we've pushed to master-solr-8621, minus the commit we wanted to revert, and also after rebasing all our changes on current 'master'. The reason I didn't push it to our branch is so that we don't need to revert that commit, and also cause at some point we merged with master rather than rebasing.

          Note that I had to remove SolrIndexConfig.buildMergePolicy() cause of a test that failed. We should just call SolrIndexConfig.buildMergePolicyFromInfo() to unify how we configure the MP.

          If you prefer that I push it to branch, then I can try and squash all the commits etc., or can push it to a separate branch. But I think it's ready.

          Show
          Shai Erera added a comment - Christine Poerschke , patch includes the changes from all commits that we've pushed to master-solr-8621, minus the commit we wanted to revert, and also after rebasing all our changes on current 'master'. The reason I didn't push it to our branch is so that we don't need to revert that commit, and also cause at some point we merged with master rather than rebasing. Note that I had to remove SolrIndexConfig.buildMergePolicy() cause of a test that failed. We should just call SolrIndexConfig.buildMergePolicyFromInfo() to unify how we configure the MP. If you prefer that I push it to branch, then I can try and squash all the commits etc., or can push it to a separate branch. But I think it's ready.
          Hide
          Christine Poerschke added a comment -

          Shai Erera, thanks for combining branch snapshot into a patch file.

          I've continued work on the solrconfig.xml test changes and from that found that just a <mergeFactor> element without a <mergePolicy> element is currently valid and SolrIndexConfig.java needs to consider it, something like https://git1-us-west.apache.org/repos/asf?p=lucene-solr.git;a=commit;h=7b21aa1c - what do you think?

          Tomorrow/Tuesday I'm hoping to tackle the tests currently containing

          <mergePolicy class="${solr.tests.mergePolicy:org.apache.solr.util.RandomMergePolicy}" />
          

          i.e. continuing on from http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/28d07935 refactor.

          If you prefer that I push it to branch ... But I think it's ready.

          Happy for you to take care of the push to master and branch_5x merge.

          In terms of ready, if the solrconfig-mergepolicy-legacy.xml i.e. TestMergePolicyConfig passes and other existing tests pass, then yes let's go for it and commit the code changes and partial test changes. Once that is done then we can take care of the remaining tests and examples and the SortingMergePolicy for SOLR-5730 can also be added then. How does that sound?

          Show
          Christine Poerschke added a comment - Shai Erera , thanks for combining branch snapshot into a patch file. I've continued work on the solrconfig.xml test changes and from that found that just a <mergeFactor> element without a <mergePolicy> element is currently valid and SolrIndexConfig.java needs to consider it, something like https://git1-us-west.apache.org/repos/asf?p=lucene-solr.git;a=commit;h=7b21aa1c - what do you think? Tomorrow/Tuesday I'm hoping to tackle the tests currently containing <mergePolicy class= "${solr.tests.mergePolicy:org.apache.solr.util.RandomMergePolicy}" /> i.e. continuing on from http://git-wip-us.apache.org/repos/asf/lucene-solr/commit/28d07935 refactor. If you prefer that I push it to branch ... But I think it's ready. Happy for you to take care of the push to master and branch_5x merge. In terms of ready, if the solrconfig-mergepolicy-legacy.xml i.e. TestMergePolicyConfig passes and other existing tests pass, then yes let's go for it and commit the code changes and partial test changes. Once that is done then we can take care of the remaining tests and examples and the SortingMergePolicy for SOLR-5730 can also be added then. How does that sound?
          Hide
          Shai Erera added a comment -

          Thanks Christine Poerschke, I'm running tests now. I'm not sure why you needed to add those checks to SolrIndexConfig and remove the assertions? The 'legacy' test that filed due to <mergeFactor> is fixed in my patch, by always calling buildMergePolicyFromInfo() (which I might change to just buildMergePolicy() now). Was there another test that failed?

          Show
          Shai Erera added a comment - Thanks Christine Poerschke , I'm running tests now. I'm not sure why you needed to add those checks to SolrIndexConfig and remove the assertions? The 'legacy' test that filed due to <mergeFactor> is fixed in my patch, by always calling buildMergePolicyFromInfo() (which I might change to just buildMergePolicy() now). Was there another test that failed?
          Hide
          ASF subversion and git services added a comment -

          Commit fc5b1ac279a02f51b634f0fd16ae3efdcdbc520b in lucene-solr's branch refs/heads/master from Shai Erera
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fc5b1ac ]

          SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFactory>

          Show
          ASF subversion and git services added a comment - Commit fc5b1ac279a02f51b634f0fd16ae3efdcdbc520b in lucene-solr's branch refs/heads/master from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fc5b1ac ] SOLR-8621 : deprecate <mergePolicy> in favor of <mergePolicyFactory>
          Hide
          ASF subversion and git services added a comment -

          Commit eac5a35960ba77f4f44ae255f658fc2174325af3 in lucene-solr's branch refs/heads/branch_5x from Shai Erera
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eac5a35 ]

          SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFactory>

          Show
          ASF subversion and git services added a comment - Commit eac5a35960ba77f4f44ae255f658fc2174325af3 in lucene-solr's branch refs/heads/branch_5x from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=eac5a35 ] SOLR-8621 : deprecate <mergePolicy> in favor of <mergePolicyFactory>
          Hide
          ASF subversion and git services added a comment -

          Commit fe2cf250796a98ff1791a504d21acb67f0a1c397 in lucene-solr's branch refs/heads/master from Shai Erera
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fe2cf25 ]

          SOLR-8621: Fix SolrIndexConfig.toMap() to use mergePolicyFactoryInfo if present

          Show
          ASF subversion and git services added a comment - Commit fe2cf250796a98ff1791a504d21acb67f0a1c397 in lucene-solr's branch refs/heads/master from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=fe2cf25 ] SOLR-8621 : Fix SolrIndexConfig.toMap() to use mergePolicyFactoryInfo if present
          Hide
          Shai Erera added a comment -

          Christine Poerschke I pushed the changes to both master and branch_5x. I think we can now remove support for <mergePolicy> in master? We also need to modify the ref guide .. do we do that via JIRA too? Let me know how you want to proceed, not sure if you've already started to work on any of these.

          BTW, I'll delete the remote tracking branch or this feature. We can create a new one if needed. Have you pushed anything more to it?

          Show
          Shai Erera added a comment - Christine Poerschke I pushed the changes to both master and branch_5x. I think we can now remove support for <mergePolicy> in master? We also need to modify the ref guide .. do we do that via JIRA too? Let me know how you want to proceed, not sure if you've already started to work on any of these. BTW, I'll delete the remote tracking branch or this feature. We can create a new one if needed. Have you pushed anything more to it?
          Hide
          ASF subversion and git services added a comment -

          Commit 71f4e01d9fa5d008cc59c0343637d8b2585018c5 in lucene-solr's branch refs/heads/master from Shai Erera
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=71f4e01 ]

          SOLR-8621: add missing package-info.java

          Show
          ASF subversion and git services added a comment - Commit 71f4e01d9fa5d008cc59c0343637d8b2585018c5 in lucene-solr's branch refs/heads/master from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=71f4e01 ] SOLR-8621 : add missing package-info.java
          Hide
          ASF subversion and git services added a comment -

          Commit 67bf26f78d588748c157445b33e6c6b78197ec98 in lucene-solr's branch refs/heads/branch_5x from Shai Erera
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=67bf26f ]

          SOLR-8621: add missing package-info.java

          Show
          ASF subversion and git services added a comment - Commit 67bf26f78d588748c157445b33e6c6b78197ec98 in lucene-solr's branch refs/heads/branch_5x from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=67bf26f ] SOLR-8621 : add missing package-info.java
          Hide
          Shawn Heisey added a comment -

          I just saw a commit email with some "upgrading from 5.4" changes for this issue.

          The example in the doc shows a maxMergeAtOnce and segmentsPerTier value of 42 ... but this specific example will probably not work entirely as expected, because the default value of "maxMergeAtOnceExplicit" is 30.

          With a value of 42 for the two primary settings, maxMergeAtOnceExplicit should be changed to 126. I cannot claim to know how everything interacts with that setting, but before I found out about it, I had trouble with optimizes when I set the other two values to 35. It did not behave as it was supposed to until I set the explicit value to 105.

          I wonder if perhaps the default setting of maxMergeAtOnceExplicit (if not manually configured) should be 3 times the current value of maxMergeAtOnce, rather than hardcoded to 30. If maxMergeAtOnceExplicit is actually configured, then it should definitely override that default.

          I've whipped up a patch. If it looks like a good idea, then it needs its own LUCENE issue.

          Show
          Shawn Heisey added a comment - I just saw a commit email with some "upgrading from 5.4" changes for this issue. The example in the doc shows a maxMergeAtOnce and segmentsPerTier value of 42 ... but this specific example will probably not work entirely as expected, because the default value of "maxMergeAtOnceExplicit" is 30. With a value of 42 for the two primary settings, maxMergeAtOnceExplicit should be changed to 126. I cannot claim to know how everything interacts with that setting, but before I found out about it, I had trouble with optimizes when I set the other two values to 35. It did not behave as it was supposed to until I set the explicit value to 105. I wonder if perhaps the default setting of maxMergeAtOnceExplicit (if not manually configured) should be 3 times the current value of maxMergeAtOnce, rather than hardcoded to 30. If maxMergeAtOnceExplicit is actually configured, then it should definitely override that default. I've whipped up a patch. If it looks like a good idea, then it needs its own LUCENE issue.
          Hide
          Christine Poerschke added a comment -

          Shai Erera - thanks for pushing changes to master and branch_5x.

          ... I think we can now remove support for <mergePolicy> in master? ...

          Could I suggest leaving support in master until 5.5 is released? Perhaps 'remove <mergePolicy> support' should be a separate JIRA even? Two main motivations for keeping the deprecated mergePolicy around for a little longer: (a) I'd like to do a little more work on the tests to ensure we didn't inadvertently break anything as part of the change. (b) If any little changes are needed then it might be easier to commit-to-master-and-then-merge-to-5.5 if master still has the deprecated mergePolicy support. How does that sound?

          We also need to modify the ref guide .. do we do that via JIRA too? Let me know how you want to proceed, not sure if you've already started to work on any of these.

          Good question, not sure how ref guide changes are normally handled. No, I haven't started to work on the ref guide things as yet.

          BTW, I'll delete the remote tracking branch or this feature. We can create a new one if needed. Have you pushed anything more to it?

          Let me check, I think there might have been some test-related stuff and draft solr/CHANGES.txt examples.

          Show
          Christine Poerschke added a comment - Shai Erera - thanks for pushing changes to master and branch_5x. ... I think we can now remove support for <mergePolicy> in master? ... Could I suggest leaving support in master until 5.5 is released? Perhaps 'remove <mergePolicy> support' should be a separate JIRA even? Two main motivations for keeping the deprecated mergePolicy around for a little longer: (a) I'd like to do a little more work on the tests to ensure we didn't inadvertently break anything as part of the change. (b) If any little changes are needed then it might be easier to commit-to-master-and-then-merge-to-5.5 if master still has the deprecated mergePolicy support. How does that sound? We also need to modify the ref guide .. do we do that via JIRA too? Let me know how you want to proceed, not sure if you've already started to work on any of these. Good question, not sure how ref guide changes are normally handled. No, I haven't started to work on the ref guide things as yet. BTW, I'll delete the remote tracking branch or this feature. We can create a new one if needed. Have you pushed anything more to it? Let me check, I think there might have been some test-related stuff and draft solr/CHANGES.txt examples.
          Hide
          Christine Poerschke added a comment -

          Shawn Heisey - It's the master-solr-8621 i.e. working branch https://git1-us-west.apache.org/repos/asf?p=lucene-solr.git;a=commitdiff;h=8ec2c844 that you refer to I think?

          ... maxMergeAtOnce and segmentsPerTier value of 42 ...

          My intention was to communicate that users with an existing <mergeFactor> or <maxMergeDocs> setting likely need to add an equivalent element to the <mergePolicyFactory> element. Using 42 as an example value was clearly distracting, I will use ?? instead then.

          ... maxMergeAtOnce ... maxMergeAtOnceExplicit ...

          Interesting point about the alignment or otherwise of these values. Yes, that sounds worthy of its own LUCENE issue.

          Show
          Christine Poerschke added a comment - Shawn Heisey - It's the master-solr-8621 i.e. working branch https://git1-us-west.apache.org/repos/asf?p=lucene-solr.git;a=commitdiff;h=8ec2c844 that you refer to I think? ... maxMergeAtOnce and segmentsPerTier value of 42 ... My intention was to communicate that users with an existing <mergeFactor> or <maxMergeDocs> setting likely need to add an equivalent element to the <mergePolicyFactory> element. Using 42 as an example value was clearly distracting, I will use ?? instead then. ... maxMergeAtOnce ... maxMergeAtOnceExplicit ... Interesting point about the alignment or otherwise of these values. Yes, that sounds worthy of its own LUCENE issue.
          Hide
          ASF subversion and git services added a comment -

          Commit 360376095446db236c1708af18b95dd13c605634 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3603760 ]

          SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now also mentions <mergeFactor> and <maxMergeDocs> deprecation, also useCompoundFile attribute/element.

          Show
          ASF subversion and git services added a comment - Commit 360376095446db236c1708af18b95dd13c605634 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3603760 ] SOLR-8621 : solr/CHANGES.txt 'Upgrading from 5.4' section now also mentions <mergeFactor> and <maxMergeDocs> deprecation, also useCompoundFile attribute/element.
          Hide
          ASF subversion and git services added a comment -

          Commit b052a8f93776ed7fef3c47764650566cfd9a358f in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b052a8f ]

          SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now also mentions <mergeFactor> and <maxMergeDocs> deprecation, also useCompoundFile attribute/element.

          Show
          ASF subversion and git services added a comment - Commit b052a8f93776ed7fef3c47764650566cfd9a358f in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b052a8f ] SOLR-8621 : solr/CHANGES.txt 'Upgrading from 5.4' section now also mentions <mergeFactor> and <maxMergeDocs> deprecation, also useCompoundFile attribute/element.
          Hide
          Shai Erera added a comment -

          Christine Poerschke: OK so let's do the following:

          • Move all solrconfig.xmls to use the new factory, except the legacy ones (used for tests). Also, let's mark somehow all the tests/code that we want to remove in master, so it will be easier to track.
          • Open an issue to remove support for <mergePolicy> in 6.0. We can make it a blocker for 6.0.
          • About the ref guide, I don't think that we work on it via JIRA. Do we modify it in confluence directly?

          About splitting the work, is there something you prefer to do? If not, I'd rather handle the existing solrconfigs than modify the ref guide. You'll probably do a better job at it than me . I will gladly help with reviews!

          Also, if there's anything else you think should be done in the context of this issue, or need help with, please let me know!

          Show
          Shai Erera added a comment - Christine Poerschke : OK so let's do the following: Move all solrconfig.xmls to use the new factory, except the legacy ones (used for tests). Also, let's mark somehow all the tests/code that we want to remove in master, so it will be easier to track. Open an issue to remove support for <mergePolicy> in 6.0. We can make it a blocker for 6.0. About the ref guide, I don't think that we work on it via JIRA. Do we modify it in confluence directly? About splitting the work, is there something you prefer to do? If not, I'd rather handle the existing solrconfigs than modify the ref guide. You'll probably do a better job at it than me . I will gladly help with reviews! Also, if there's anything else you think should be done in the context of this issue, or need help with, please let me know!
          Hide
          ASF subversion and git services added a comment -

          Commit e9c90037aaf2f392d7f99a7837abd944a9577e9e in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e9c9003 ]

          SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now randomly chooses between solrconfig-tieredmergepolicy.xml and solrconfig-tieredmergepolicyfactory.xml; solrconfig-tieredmergepolicyfactory.xml fix so that TestMergePolicyConfig.testTieredMergePolicyConfig passes.

          Show
          ASF subversion and git services added a comment - Commit e9c90037aaf2f392d7f99a7837abd944a9577e9e in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e9c9003 ] SOLR-8621 : TestMergePolicyConfig.testTieredMergePolicyConfig now randomly chooses between solrconfig-tieredmergepolicy.xml and solrconfig-tieredmergepolicyfactory.xml; solrconfig-tieredmergepolicyfactory.xml fix so that TestMergePolicyConfig.testTieredMergePolicyConfig passes.
          Hide
          Christine Poerschke added a comment -

          About splitting the work, is there something you prefer to do?

          Could I take on the solr/core/src/test-files/solr/collection1/conf portion of the solrconfig.xml changes? It continues from the remaining bits of the master-solr-8621 working branch that weren't yet ready to be committed alongside the main changes.

          BTW, I'll delete the remote tracking branch or this feature. We can create a new one if needed. Have you pushed anything more to it?

          From the things pushed late yesterday and not included in the main commit, I have squashed/rebased stuff locally and captured all I need i.e. the master-solr-8621 working branch as-is can be deleted. Not sure if we need or want a new one to replace it?

          Show
          Christine Poerschke added a comment - About splitting the work, is there something you prefer to do? Could I take on the solr/core/src/test-files/solr/collection1/conf portion of the solrconfig.xml changes? It continues from the remaining bits of the master-solr-8621 working branch that weren't yet ready to be committed alongside the main changes. BTW, I'll delete the remote tracking branch or this feature. We can create a new one if needed. Have you pushed anything more to it? From the things pushed late yesterday and not included in the main commit, I have squashed/rebased stuff locally and captured all I need i.e. the master-solr-8621 working branch as-is can be deleted. Not sure if we need or want a new one to replace it?
          Hide
          Shai Erera added a comment -

          Could I take on the

          Sure. You lead this, I'm only here to help . So tell me how can I help more.

          Not sure if we need or want a new one to replace it?

          For fixing the config xmls, I don't think that we need a branch. If we're not collaborating on the same task / code path, no need for a remote branch. If that's OK with you, I'll go ahead and delete it.

          Show
          Shai Erera added a comment - Could I take on the Sure. You lead this, I'm only here to help . So tell me how can I help more. Not sure if we need or want a new one to replace it? For fixing the config xmls, I don't think that we need a branch. If we're not collaborating on the same task / code path, no need for a remote branch. If that's OK with you, I'll go ahead and delete it.
          Hide
          ASF subversion and git services added a comment -

          Commit 39fd942514a5474d81a285a3a69f4d2c1dca33c7 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=39fd942 ]

          SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now randomly chooses between solrconfig-tieredmergepolicy.xml and solrconfig-tieredmergepolicyfactory.xml; solrconfig-tieredmergepolicyfactory.xml fix so that TestMergePolicyConfig.testTieredMergePolicyConfig passes.

          Show
          ASF subversion and git services added a comment - Commit 39fd942514a5474d81a285a3a69f4d2c1dca33c7 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=39fd942 ] SOLR-8621 : TestMergePolicyConfig.testTieredMergePolicyConfig now randomly chooses between solrconfig-tieredmergepolicy.xml and solrconfig-tieredmergepolicyfactory.xml; solrconfig-tieredmergepolicyfactory.xml fix so that TestMergePolicyConfig.testTieredMergePolicyConfig passes.
          Hide
          Christine Poerschke added a comment -

          It's great to collaborate on this and I agree if we don't work on the same code paths then no need for a remote branch. Yes please, go ahead and delete the master-solr-8621 working branch.

          Could you take care of the solr/contrib and solr/example solrconfig.xml changes? And opening the follow-on issue to remove support for <mergePolicy> in 6.0?

          My next steps are to continue on the core/src/test-files/solr/collection1/conf solrconfig.xml changes and in parallel with that to rebase SOLR-5730 where a SortingMergePolicyFactory code review would be much appreciated later on.

          Show
          Christine Poerschke added a comment - It's great to collaborate on this and I agree if we don't work on the same code paths then no need for a remote branch. Yes please, go ahead and delete the master-solr-8621 working branch. Could you take care of the solr/contrib and solr/example solrconfig.xml changes? And opening the follow-on issue to remove support for <mergePolicy> in 6.0? My next steps are to continue on the core/src/test-files/solr/collection1/conf solrconfig.xml changes and in parallel with that to rebase SOLR-5730 where a SortingMergePolicyFactory code review would be much appreciated later on.
          Hide
          Christine Poerschke added a comment -

          Hmm, small hiccup re: MergePolicyFactory's getMergePolicy signature. The SortingMergePolicyFactory in SOLR-5730 needs access to the schema with the 'obvious' signature change being

          -  public abstract MergePolicy getMergePolicy();
          +  public abstract MergePolicy getMergePolicy(IndexSchema schema);
          

          for the abstract MergePolicyFactory class and all classes inheriting from it.

          Shai Erera - would you have any thoughts on this? I'm thinking that changing the signature at this point is still okay since <mergePolicyFactory> support is not yet released and in practice SolrIndexConfig.buildMergePolicy(final IndexSchema schema) is always able to pass the schema to the getMergePolicy method.

          Show
          Christine Poerschke added a comment - Hmm, small hiccup re: MergePolicyFactory 's getMergePolicy signature. The SortingMergePolicyFactory in SOLR-5730 needs access to the schema with the 'obvious' signature change being - public abstract MergePolicy getMergePolicy(); + public abstract MergePolicy getMergePolicy(IndexSchema schema); for the abstract MergePolicyFactory class and all classes inheriting from it. Shai Erera - would you have any thoughts on this? I'm thinking that changing the signature at this point is still okay since <mergePolicyFactory> support is not yet released and in practice SolrIndexConfig.buildMergePolicy(final IndexSchema schema) is always able to pass the schema to the getMergePolicy method.
          Hide
          Christine Poerschke added a comment -

          Upon further consideration, changing the getMergePolicy signature means that getMergePolicyInstance and getDefaultWrappedMergePolicy and getWrappedMergePolicy signatures all change also.

          Here's an alternative, change the MergePolicyFactory constructor:

          -  protected MergePolicyFactory(SolrResourceLoader resourceLoader, MergePolicyFactoryArgs args) {
          +  protected MergePolicyFactory(SolrResourceLoader resourceLoader, MergePolicyFactoryArgs args, IndexSchema schema) {
          
          Show
          Christine Poerschke added a comment - Upon further consideration, changing the getMergePolicy signature means that getMergePolicyInstance and getDefaultWrappedMergePolicy and getWrappedMergePolicy signatures all change also. Here's an alternative, change the MergePolicyFactory constructor: - protected MergePolicyFactory(SolrResourceLoader resourceLoader, MergePolicyFactoryArgs args) { + protected MergePolicyFactory(SolrResourceLoader resourceLoader, MergePolicyFactoryArgs args, IndexSchema schema) {
          Hide
          ASF subversion and git services added a comment -

          Commit b47eeb2bb39e94f23d64df3c4621e7303bbe9dd9 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b47eeb2 ]

          SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test coverage

          • added bad-mpf-solrconfig.xml as MergePolicyFactory equivalent of bad-mp-solrconfig.xml (with DummyMergePolicyFactory as equivalent to DummyMergePolicy)
          • added solrconfig-logmergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-logmergepolicy.xml
          • added solrconfig-mergepolicyfactory-nocfs.xml as MergePolicyFactory equivalent of solrconfig-mergepolicy-nocfs.xml
          • added solrconfig-indexconfig-mergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-indexconfig.xml
          • added solrconfig-warmer-randommergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-warmer.xml
          Show
          ASF subversion and git services added a comment - Commit b47eeb2bb39e94f23d64df3c4621e7303bbe9dd9 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b47eeb2 ] SOLR-8621 : more <mergePolicy> vs. <mergePolicyFactory> test coverage added bad-mpf-solrconfig.xml as MergePolicyFactory equivalent of bad-mp-solrconfig.xml (with DummyMergePolicyFactory as equivalent to DummyMergePolicy) added solrconfig-logmergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-logmergepolicy.xml added solrconfig-mergepolicyfactory-nocfs.xml as MergePolicyFactory equivalent of solrconfig-mergepolicy-nocfs.xml added solrconfig-indexconfig-mergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-indexconfig.xml added solrconfig-warmer-randommergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-warmer.xml
          Hide
          ASF subversion and git services added a comment -

          Commit 594145f2d5307ebd208dd9d054f5b63678bd415f in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=594145f ]

          SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test coverage

          • added bad-mpf-solrconfig.xml as MergePolicyFactory equivalent of bad-mp-solrconfig.xml (with DummyMergePolicyFactory as equivalent to DummyMergePolicy)
          • added solrconfig-logmergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-logmergepolicy.xml
          • added solrconfig-mergepolicyfactory-nocfs.xml as MergePolicyFactory equivalent of solrconfig-mergepolicy-nocfs.xml
          • added solrconfig-indexconfig-mergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-indexconfig.xml
          • added solrconfig-warmer-randommergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-warmer.xml
          Show
          ASF subversion and git services added a comment - Commit 594145f2d5307ebd208dd9d054f5b63678bd415f in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=594145f ] SOLR-8621 : more <mergePolicy> vs. <mergePolicyFactory> test coverage added bad-mpf-solrconfig.xml as MergePolicyFactory equivalent of bad-mp-solrconfig.xml (with DummyMergePolicyFactory as equivalent to DummyMergePolicy) added solrconfig-logmergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-logmergepolicy.xml added solrconfig-mergepolicyfactory-nocfs.xml as MergePolicyFactory equivalent of solrconfig-mergepolicy-nocfs.xml added solrconfig-indexconfig-mergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-indexconfig.xml added solrconfig-warmer-randommergepolicyfactory.xml as MergePolicyFactory equivalent of solrconfig-warmer.xml
          Hide
          Christine Poerschke added a comment -

          e0b2ebf9c5b9013c8ab2a10717feefd11a101bcb on jira/solr-5730-master working-branch turned out as my preferred way of making the IndexSchema available to SortingMergePolicyFactory but if the alternative of

          -  public abstract MergePolicy getMergePolicy();
          +  public abstract MergePolicy getMergePolicy(IndexSchema schema);
          

          is considered more suitable then it's something that can be easily changed tomorrow/Wednesday. Shai Erera - what do you think?

          Show
          Christine Poerschke added a comment - e0b2ebf9c5b9013c8ab2a10717feefd11a101bcb on jira/solr-5730-master working-branch turned out as my preferred way of making the IndexSchema available to SortingMergePolicyFactory but if the alternative of - public abstract MergePolicy getMergePolicy(); + public abstract MergePolicy getMergePolicy(IndexSchema schema); is considered more suitable then it's something that can be easily changed tomorrow/Wednesday. Shai Erera - what do you think?
          Hide
          Shai Erera added a comment -

          Could you take care of the solr/contrib and solr/example solrconfig.xml changes?

          Sure, I'll take a look at them.

          And opening the follow-on issue to remove support for <mergePolicy> in 6.0?

          Opened SOLR-8668.

          About IndexSchema, I was going to propose that you add it as a ctor argument, but I see you've already done that. Just wondering though, what does SortingMergePolicyFactory need from IndexSchema that it cannot create on its own? It already receives the sort-by fields and order in the config, all it needs to do is to create a Sort class. What does it get from IndexSchema then?

          Show
          Shai Erera added a comment - Could you take care of the solr/contrib and solr/example solrconfig.xml changes? Sure, I'll take a look at them. And opening the follow-on issue to remove support for <mergePolicy> in 6.0? Opened SOLR-8668 . About IndexSchema , I was going to propose that you add it as a ctor argument, but I see you've already done that. Just wondering though, what does SortingMergePolicyFactory need from IndexSchema that it cannot create on its own? It already receives the sort-by fields and order in the config, all it needs to do is to create a Sort class. What does it get from IndexSchema then?
          Hide
          Shai Erera added a comment -

          Patch changes solrconfig.xmls under solr/contrib and solr/example. Christine Poerschke, none of these files actually configured an MP, so I only changed the commented out sections. Let me know if you have concerns about this change.

          Show
          Shai Erera added a comment - Patch changes solrconfig.xmls under solr/contrib and solr/example. Christine Poerschke , none of these files actually configured an MP, so I only changed the commented out sections. Let me know if you have concerns about this change.
          Hide
          Christine Poerschke added a comment -

          Shai Erera - thanks for patch changes to solrconfig.xmls under solr/contrib and solr/example. Question: if/where the commented out sections correspond completely to what is 'default merge policy factory' might it make sense to just completely remove the commented out <mergePolicy*> element and just have a comment saying that the default will be used because the <mergePolicyFactory> element is absent but something else could be configured via

          <mergePolicyFacory class="...">
          </mergePolicyFacory>
          

          if required? That would be my preference I think. Alternatively, if we are keeping and just updating the commented out elements then the <mergeFactor> element maps to TieredMergePolicyFactory.maxMergeAtOnce and TieredMergePolicyFactory.segmentsPerTier but TieredMergePolicy[Factory] itself has no mergeFactor setter. With solrconfig-tieredmergepolicyfactory.xml I got this wrong also at the first attempt and thus made 360376095446db236c1708af18b95dd13c605634 solr/CHANGES.txt 'Upgrading from 5.4' section change. What do you think?

          Show
          Christine Poerschke added a comment - Shai Erera - thanks for patch changes to solrconfig.xmls under solr/contrib and solr/example. Question: if/where the commented out sections correspond completely to what is 'default merge policy factory' might it make sense to just completely remove the commented out <mergePolicy*> element and just have a comment saying that the default will be used because the <mergePolicyFactory> element is absent but something else could be configured via <mergePolicyFacory class= "..." > </mergePolicyFacory> if required? That would be my preference I think. Alternatively, if we are keeping and just updating the commented out elements then the <mergeFactor> element maps to TieredMergePolicyFactory.maxMergeAtOnce and TieredMergePolicyFactory.segmentsPerTier but TieredMergePolicy[Factory] itself has no mergeFactor setter. With solrconfig-tieredmergepolicyfactory.xml I got this wrong also at the first attempt and thus made 360376095446db236c1708af18b95dd13c605634 solr/CHANGES.txt 'Upgrading from 5.4' section change. What do you think?
          Hide
          Christine Poerschke added a comment -

          ... what does SortingMergePolicyFactory need from IndexSchema that it cannot create on its own? It already receives the sort-by fields and order in the config, all it needs to do is to create a Sort class. What does it get from IndexSchema then?

          The plan so far was that the sort would be specified as a string e.g.

          <str name="sort">popularity desc, timestamp desc</str>
          

          in the same way as when a &sort=... parameter is passed as part of a select request. The solrconfig.xml SortingMergePolicyFactory's sort string is then parsed using the essentially the same methods used for parsing the sort parameter in the select request e.g. SortSpecParsing.parseSortSpec(sortArg, schema) - does that kind of make sense?

          ... It already receives the sort-by fields and order in the config, all it needs to do is to create a Sort class ...

          Are you saying that perhaps there is a way to create the Sort class without use of IndexSchema if the solrconfig.xml sort-by fields and order are provided in a structured way that requires no parsing? Interesting idea, let me look into that a little further.

          Show
          Christine Poerschke added a comment - ... what does SortingMergePolicyFactory need from IndexSchema that it cannot create on its own? It already receives the sort-by fields and order in the config, all it needs to do is to create a Sort class. What does it get from IndexSchema then? The plan so far was that the sort would be specified as a string e.g. <str name= "sort" >popularity desc, timestamp desc</str> in the same way as when a &sort=... parameter is passed as part of a select request. The solrconfig.xml SortingMergePolicyFactory's sort string is then parsed using the essentially the same methods used for parsing the sort parameter in the select request e.g. SortSpecParsing.parseSortSpec(sortArg, schema) - does that kind of make sense? ... It already receives the sort-by fields and order in the config, all it needs to do is to create a Sort class ... Are you saying that perhaps there is a way to create the Sort class without use of IndexSchema if the solrconfig.xml sort-by fields and order are provided in a structured way that requires no parsing? Interesting idea, let me look into that a little further.
          Hide
          Christine Poerschke added a comment -

          ticket cross-reference: LUCENE-7020 is the follow-on ticket.

          Show
          Christine Poerschke added a comment - ticket cross-reference: LUCENE-7020 is the follow-on ticket.
          Hide
          Christine Poerschke added a comment -

          ticket cross-reference: LUCENE-7020 is the follow-on ticket.

          Show
          Christine Poerschke added a comment - ticket cross-reference: LUCENE-7020 is the follow-on ticket.
          Hide
          Shai Erera added a comment -

          Good catch, I just blindly moved that 'mergeFactor' in . I'll remove it. I'd like to keep the example though, since it's consistent with the rest of the commented out examples.

          About SortingMergePolicyFactory and IndexSchema, I see that SortSpecParsing uses the provided schema only to validate that the field exists in the schema. If we want to keep that validity check, then let's continue with your proposal of passing IndexSchema to the factory's ctor.

          About this parsing logic, it also relies on request params, so perhaps factor out the parsing logic to a utility that you can use? That utility can then also validate that the sort field exists in the schema. I didn't review that method fully, but I hope it's doable.

          Show
          Shai Erera added a comment - Good catch, I just blindly moved that 'mergeFactor' in . I'll remove it. I'd like to keep the example though, since it's consistent with the rest of the commented out examples. About SortingMergePolicyFactory and IndexSchema , I see that SortSpecParsing uses the provided schema only to validate that the field exists in the schema. If we want to keep that validity check, then let's continue with your proposal of passing IndexSchema to the factory's ctor. About this parsing logic, it also relies on request params, so perhaps factor out the parsing logic to a utility that you can use? That utility can then also validate that the sort field exists in the schema. I didn't review that method fully, but I hope it's doable.
          Hide
          Shai Erera added a comment -

          Christine Poerschke if you're OK with this patch, I'll commit it.

          Show
          Shai Erera added a comment - Christine Poerschke if you're OK with this patch, I'll commit it.
          Hide
          Christine Poerschke added a comment -

          About this parsing logic, it also relies on request params, so perhaps factor out the parsing logic to a utility that you can use? That utility can then also validate that the sort field exists in the schema. I didn't review that method fully, but I hope it's doable.

          SOLR-8321 was to 'add a (SolrQueryRequest free) SortSpecParsing.parseSortSpec variant' i.e. I hope it's already done what was doable

          Revised SOLR-8621-example_contrib_configs.patch looks good to me, thanks for taking care of it.

          Show
          Christine Poerschke added a comment - About this parsing logic, it also relies on request params, so perhaps factor out the parsing logic to a utility that you can use? That utility can then also validate that the sort field exists in the schema. I didn't review that method fully, but I hope it's doable. SOLR-8321 was to 'add a (SolrQueryRequest free) SortSpecParsing.parseSortSpec variant' i.e. I hope it's already done what was doable Revised SOLR-8621 -example_contrib_configs.patch looks good to me, thanks for taking care of it.
          Hide
          ASF subversion and git services added a comment -

          Commit a5accccbfcb95ee180d0f069274c07f771844f61 in lucene-solr's branch refs/heads/master from Shai Erera
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a5acccc ]

          SOLR-8621: fix solrconfig.xml under contrib and example

          Show
          ASF subversion and git services added a comment - Commit a5accccbfcb95ee180d0f069274c07f771844f61 in lucene-solr's branch refs/heads/master from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a5acccc ] SOLR-8621 : fix solrconfig.xml under contrib and example
          Hide
          ASF subversion and git services added a comment -

          Commit 24f4025dc13aaaea7e89e349249e8819ba925279 in lucene-solr's branch refs/heads/branch_5x from Shai Erera
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=24f4025 ]

          SOLR-8621: fix solrconfig.xml under contrib and example

          Show
          ASF subversion and git services added a comment - Commit 24f4025dc13aaaea7e89e349249e8819ba925279 in lucene-solr's branch refs/heads/branch_5x from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=24f4025 ] SOLR-8621 : fix solrconfig.xml under contrib and example
          Hide
          Shai Erera added a comment -

          You're right Christine, I didn't notice the IndexSchema only variant. Let's continue on SOLR-5730 then by adding the schema to the factory's ctor.

          So besides this API change, what's left to do in the context of this issue? Since it's marked blocker for 5.5, I want to make sure that we don't hold up the release too long:

          • So the API change has to go in.
          • Adding tests can be done separately? (unless you already have some work done there).
          • We finished (as far as I could tell), updating all existing solrconfig.xmls.
          • We have a separate issue to remove support in 6.0.
          • We should update the ref guide, but the ref guide is usually released after the binaries anyway.

          Am I missing something?

          Show
          Shai Erera added a comment - You're right Christine, I didn't notice the IndexSchema only variant. Let's continue on SOLR-5730 then by adding the schema to the factory's ctor. So besides this API change, what's left to do in the context of this issue? Since it's marked blocker for 5.5, I want to make sure that we don't hold up the release too long: So the API change has to go in. Adding tests can be done separately? (unless you already have some work done there). We finished (as far as I could tell), updating all existing solrconfig.xmls. We have a separate issue to remove support in 6.0. We should update the ref guide, but the ref guide is usually released after the binaries anyway. Am I missing something?
          Hide
          ASF subversion and git services added a comment -

          Commit 5d32609cdc413e15619a94d8d508987a65514e7e in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5d32609 ]

          SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor

          Show
          ASF subversion and git services added a comment - Commit 5d32609cdc413e15619a94d8d508987a65514e7e in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5d32609 ] SOLR-8621 : add IndexSchema arg to MergePolicyFactory constructor
          Hide
          Christine Poerschke added a comment -

          ... Am I missing something?

          No, that list of bullet points sounds right to me. Additional/Remaining test coverage to be done separately, yes, that makes sense and I hope that by end of today we can 'un-blocker' for 5.5 here. Before then, I seem to have discovered issues with the (wrapper?) factory and the setters i.e. them not taking effect as expected, hoping to have test case and fix shortly.

          Show
          Christine Poerschke added a comment - ... Am I missing something? No, that list of bullet points sounds right to me. Additional/Remaining test coverage to be done separately, yes, that makes sense and I hope that by end of today we can 'un-blocker' for 5.5 here. Before then, I seem to have discovered issues with the (wrapper?) factory and the setters i.e. them not taking effect as expected, hoping to have test case and fix shortly.
          Hide
          ASF subversion and git services added a comment -

          Commit 5d106503e7d4fbb8ac015c4fc723883f4ab7397e in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5d10650 ]

          SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor

          Show
          ASF subversion and git services added a comment - Commit 5d106503e7d4fbb8ac015c4fc723883f4ab7397e in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=5d10650 ] SOLR-8621 : add IndexSchema arg to MergePolicyFactory constructor
          Hide
          ASF subversion and git services added a comment -

          Commit 588e3ff0842a5d021cff09aa72d94b0b5de45ca9 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=588e3ff ]

          SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in comments in solr/contrib and solr/example solrconfig.xml files.

          Show
          ASF subversion and git services added a comment - Commit 588e3ff0842a5d021cff09aa72d94b0b5de45ca9 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=588e3ff ] SOLR-8621 : fix mergePolicyFacory vs. mergePolicyFactory typos in comments in solr/contrib and solr/example solrconfig.xml files.
          Hide
          ASF subversion and git services added a comment -

          Commit bbbc90f58b2f336c4c51f4844cd0f63121c76ccf in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bbbc90f ]

          SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in comments in solr/contrib and solr/example solrconfig.xml files.

          Show
          ASF subversion and git services added a comment - Commit bbbc90f58b2f336c4c51f4844cd0f63121c76ccf in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bbbc90f ] SOLR-8621 : fix mergePolicyFacory vs. mergePolicyFactory typos in comments in solr/contrib and solr/example solrconfig.xml files.
          Hide
          ASF subversion and git services added a comment -

          Commit 6b6932e8e1f72caf29a078f0532a56c284711f9f in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b6932e ]

          SOLR-8621: WrapperMergePolicyFactory logic tweaks

          • fix so that getMergePolicy() can now be called more than once
          • added WrapperMergePolicyFactoryTest.testUpgradeIndexMergePolicyFactory()
          • account for overlap between wrapping and wrapped setters (and disallow it)
            • illustration:
              <mergePolicyFactory class="UpgradeMergePolicyFactory">
              <int name="noCFSRatio">0.24</int>
              <str name="wrapped.prefix">mergePolicy</str>
              <str name="mergePolicy.class">TieredMergePolicyFactory</str>
              <int name="mergePolicy.noCFSRatio">0.42</int>
              </mergePolicyFactory>
            • implementation details: the wrapping MP's setter calls the wrapped MP's setter and in the current code the wrapping MP's value prevails i.e. the 0.24 value in the illustration since the wrapped MP is constructed before the wrapping MP. an end-user however might reasonably assume that the wrapped MP's 0.42 value will prevail. at best configuring the same setter twice within the same overall <mergePolicyFactory> element is ambiguous and so the code now disallows it.
          Show
          ASF subversion and git services added a comment - Commit 6b6932e8e1f72caf29a078f0532a56c284711f9f in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6b6932e ] SOLR-8621 : WrapperMergePolicyFactory logic tweaks fix so that getMergePolicy() can now be called more than once added WrapperMergePolicyFactoryTest.testUpgradeIndexMergePolicyFactory() account for overlap between wrapping and wrapped setters (and disallow it) illustration: <mergePolicyFactory class="UpgradeMergePolicyFactory"> <int name="noCFSRatio">0.24</int> <str name="wrapped.prefix">mergePolicy</str> <str name="mergePolicy.class">TieredMergePolicyFactory</str> <int name="mergePolicy.noCFSRatio">0.42</int> </mergePolicyFactory> implementation details: the wrapping MP's setter calls the wrapped MP's setter and in the current code the wrapping MP's value prevails i.e. the 0.24 value in the illustration since the wrapped MP is constructed before the wrapping MP. an end-user however might reasonably assume that the wrapped MP's 0.42 value will prevail. at best configuring the same setter twice within the same overall <mergePolicyFactory> element is ambiguous and so the code now disallows it.
          Hide
          Christine Poerschke added a comment -

          Shai Erera - would you have any thoughts re: 6b6932e8e1f72caf29a078f0532a56c284711f9f commit above? If it is logical and reasonable then I will cherry-pick it to branch_5x and branch_5_5 tomorrow/Thursday. Thanks!

          Show
          Christine Poerschke added a comment - Shai Erera - would you have any thoughts re: 6b6932e8e1f72caf29a078f0532a56c284711f9f commit above? If it is logical and reasonable then I will cherry-pick it to branch_5x and branch_5_5 tomorrow/Thursday. Thanks!
          Hide
          Shai Erera added a comment -

          Christine Poerschke thx for fixing that typo! And your latest commit looks fine to me. +1 to get it in.

          Show
          Shai Erera added a comment - Christine Poerschke thx for fixing that typo! And your latest commit looks fine to me. +1 to get it in.
          Hide
          Christine Poerschke added a comment -

          Shai Erera - thanks for reviewing the latest commit, i'll cherry-pick it to branch_5x and branch_5_5 later today.

          No problem about the typo, I had also missed it in the patches and just stumbled across it when git-grep-ing to see which places still mention mergePolicy-but-not-mergePolicyFactory in their solrconfig.xml file.

          Somewhat but not quite related to MergePolicyFactory here and SortingMergePolicyFactory on SOLR-5730 - there were a few more TestSortingMergePolicy.testForceMergeNotNeeded test failures after LUCENE-7010 and maybe or maybe not similar to LUCENE-7008 test failure - would you have bandwidth to take a look perhaps at some point?

          Show
          Christine Poerschke added a comment - Shai Erera - thanks for reviewing the latest commit, i'll cherry-pick it to branch_5x and branch_5_5 later today. No problem about the typo, I had also missed it in the patches and just stumbled across it when git-grep-ing to see which places still mention mergePolicy-but-not-mergePolicyFactory in their solrconfig.xml file. Somewhat but not quite related to MergePolicyFactory here and SortingMergePolicyFactory on SOLR-5730 - there were a few more TestSortingMergePolicy.testForceMergeNotNeeded test failures after LUCENE-7010 and maybe or maybe not similar to LUCENE-7008 test failure - would you have bandwidth to take a look perhaps at some point?
          Hide
          Shai Erera added a comment -

          Can you point me to a test failure?

          Show
          Shai Erera added a comment - Can you point me to a test failure?
          Hide
          Christine Poerschke added a comment -

          Sure, there were two failures - https://builds.apache.org/job/Lucene-Solr-NightlyTests-trunk/926/ on Feb 6th and http://jenkins.thetaphi.de/job/Lucene-Solr-5.x-Linux/15524/ on Feb 9th - I double-counted the latter one and thus arrived at the impression of 'two' failures being 'a few', sorry.

          Show
          Christine Poerschke added a comment - Sure, there were two failures - https://builds.apache.org/job/Lucene-Solr-NightlyTests-trunk/926/ on Feb 6th and http://jenkins.thetaphi.de/job/Lucene-Solr-5.x-Linux/15524/ on Feb 9th - I double-counted the latter one and thus arrived at the impression of 'two' failures being 'a few', sorry.
          Hide
          Shai Erera added a comment -

          Thanks I'll take a look.

          Show
          Shai Erera added a comment - Thanks I'll take a look.
          Hide
          ASF subversion and git services added a comment -

          Commit bdef478737792afb861dc73ca33cb9a1775af4fe in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bdef478 ]

          SOLR-8621: WrapperMergePolicyFactory logic tweaks

          • fix so that getMergePolicy() can now be called more than once
          • added WrapperMergePolicyFactoryTest.testUpgradeIndexMergePolicyFactory()
          • account for overlap between wrapping and wrapped setters (and disallow it)
            • illustration:
              <mergePolicyFactory class="UpgradeMergePolicyFactory">
              <int name="noCFSRatio">0.24</int>
              <str name="wrapped.prefix">mergePolicy</str>
              <str name="mergePolicy.class">TieredMergePolicyFactory</str>
              <int name="mergePolicy.noCFSRatio">0.42</int>
              </mergePolicyFactory>
            • implementation details: the wrapping MP's setter calls the wrapped MP's setter and in the current code the wrapping MP's value prevails i.e. the 0.24 value in the illustration since the wrapped MP is constructed before the wrapping MP. an end-user however might reasonably assume that the wrapped MP's 0.42 value will prevail. at best configuring the same setter twice within the same overall <mergePolicyFactory> element is ambiguous and so the code now disallows it.
          Show
          ASF subversion and git services added a comment - Commit bdef478737792afb861dc73ca33cb9a1775af4fe in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=bdef478 ] SOLR-8621 : WrapperMergePolicyFactory logic tweaks fix so that getMergePolicy() can now be called more than once added WrapperMergePolicyFactoryTest.testUpgradeIndexMergePolicyFactory() account for overlap between wrapping and wrapped setters (and disallow it) illustration: <mergePolicyFactory class="UpgradeMergePolicyFactory"> <int name="noCFSRatio">0.24</int> <str name="wrapped.prefix">mergePolicy</str> <str name="mergePolicy.class">TieredMergePolicyFactory</str> <int name="mergePolicy.noCFSRatio">0.42</int> </mergePolicyFactory> implementation details: the wrapping MP's setter calls the wrapped MP's setter and in the current code the wrapping MP's value prevails i.e. the 0.24 value in the illustration since the wrapped MP is constructed before the wrapping MP. an end-user however might reasonably assume that the wrapped MP's 0.42 value will prevail. at best configuring the same setter twice within the same overall <mergePolicyFactory> element is ambiguous and so the code now disallows it.
          Hide
          ASF subversion and git services added a comment -

          Commit 47055d569dbefb41f88631ad917ab76b550ad8d4 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=47055d5 ]

          SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in comments in solr/contrib and solr/example solrconfig.xml files.

          Show
          ASF subversion and git services added a comment - Commit 47055d569dbefb41f88631ad917ab76b550ad8d4 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=47055d5 ] SOLR-8621 : fix mergePolicyFacory vs. mergePolicyFactory typos in comments in solr/contrib and solr/example solrconfig.xml files.
          Hide
          ASF subversion and git services added a comment -

          Commit 3f06f9a76f97203445ff9b35306ec0dcda59e0d8 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3f06f9a ]

          SOLR-8621: WrapperMergePolicyFactory logic tweaks

          • fix so that getMergePolicy() can now be called more than once
          • added WrapperMergePolicyFactoryTest.testUpgradeIndexMergePolicyFactory()
          • account for overlap between wrapping and wrapped setters (and disallow it)
            • illustration:
              <mergePolicyFactory class="UpgradeMergePolicyFactory">
              <int name="noCFSRatio">0.24</int>
              <str name="wrapped.prefix">mergePolicy</str>
              <str name="mergePolicy.class">TieredMergePolicyFactory</str>
              <int name="mergePolicy.noCFSRatio">0.42</int>
              </mergePolicyFactory>
            • implementation details: the wrapping MP's setter calls the wrapped MP's setter and in the current code the wrapping MP's value prevails i.e. the 0.24 value in the illustration since the wrapped MP is constructed before the wrapping MP. an end-user however might reasonably assume that the wrapped MP's 0.42 value will prevail. at best configuring the same setter twice within the same overall <mergePolicyFactory> element is ambiguous and so the code now disallows it.
          Show
          ASF subversion and git services added a comment - Commit 3f06f9a76f97203445ff9b35306ec0dcda59e0d8 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=3f06f9a ] SOLR-8621 : WrapperMergePolicyFactory logic tweaks fix so that getMergePolicy() can now be called more than once added WrapperMergePolicyFactoryTest.testUpgradeIndexMergePolicyFactory() account for overlap between wrapping and wrapped setters (and disallow it) illustration: <mergePolicyFactory class="UpgradeMergePolicyFactory"> <int name="noCFSRatio">0.24</int> <str name="wrapped.prefix">mergePolicy</str> <str name="mergePolicy.class">TieredMergePolicyFactory</str> <int name="mergePolicy.noCFSRatio">0.42</int> </mergePolicyFactory> implementation details: the wrapping MP's setter calls the wrapped MP's setter and in the current code the wrapping MP's value prevails i.e. the 0.24 value in the illustration since the wrapped MP is constructed before the wrapping MP. an end-user however might reasonably assume that the wrapped MP's 0.42 value will prevail. at best configuring the same setter twice within the same overall <mergePolicyFactory> element is ambiguous and so the code now disallows it.
          Hide
          ASF subversion and git services added a comment -

          Commit 360051a414e291a7b3ffb5a0180a404fa18f3a6c in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=360051a ]

          SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method

          Here in SOLR-8621:

          • UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory
          • (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory

          Elsewhere in SOLR-5730:

          • SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Show
          ASF subversion and git services added a comment - Commit 360051a414e291a7b3ffb5a0180a404fa18f3a6c in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=360051a ] SOLR-8621 : factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method Here in SOLR-8621 : UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory Elsewhere in SOLR-5730 : SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Hide
          ASF subversion and git services added a comment -

          Commit 360051a414e291a7b3ffb5a0180a404fa18f3a6c in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=360051a ]

          SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method

          Here in SOLR-8621:

          • UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory
          • (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory

          Elsewhere in SOLR-5730:

          • SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Show
          ASF subversion and git services added a comment - Commit 360051a414e291a7b3ffb5a0180a404fa18f3a6c in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=360051a ] SOLR-8621 : factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method Here in SOLR-8621 : UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory Elsewhere in SOLR-5730 : SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Hide
          ASF subversion and git services added a comment -

          Commit b2e47984f4411ec3cdde93ae57f72daab789b254 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b2e4798 ]

          SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for existing (test-framework) RandomForceMergePolicy

          Show
          ASF subversion and git services added a comment - Commit b2e47984f4411ec3cdde93ae57f72daab789b254 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b2e4798 ] SOLR-8621 : add (test-framework) RandomForceMergePolicyFactory for existing (test-framework) RandomForceMergePolicy
          Hide
          ASF subversion and git services added a comment -

          Commit 159ace1b7c6d002dd30e3fd17f497f3093944039 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=159ace1 ]

          SOLR-8621: solrconfig.xml in solr/server/solr/configsets and test-files/solr/configsets/bad-mergepolicy now also use <mergePolicyFactory> instead of <mergePolicy>

          Show
          ASF subversion and git services added a comment - Commit 159ace1b7c6d002dd30e3fd17f497f3093944039 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=159ace1 ] SOLR-8621 : solrconfig.xml in solr/server/solr/configsets and test-files/solr/configsets/bad-mergepolicy now also use <mergePolicyFactory> instead of <mergePolicy>
          Hide
          Christine Poerschke added a comment -

          Shai Erera - thanks for your review comments on SOLR-5730. The first two of the above three latest master commits for this SOLR-8621 ticket here are based on that. If both those two and the third master commits are reasonable then I will cherry-pick them to branch_5x and branch_5_5 tomorrow/Friday hopefully.

          For the remaining test-related changes I have created SOLR-8674 'transition from solr.tests.mergePolicy to solr.tests.mergePolicyFactory' ticket. So then we are done with SOLR-8621 here I think - what do you think?

          (The Solr Reference Guide changes still to be done separately and likely directly in Confluence.)

          Show
          Christine Poerschke added a comment - Shai Erera - thanks for your review comments on SOLR-5730 . The first two of the above three latest master commits for this SOLR-8621 ticket here are based on that. If both those two and the third master commits are reasonable then I will cherry-pick them to branch_5x and branch_5_5 tomorrow/Friday hopefully. For the remaining test-related changes I have created SOLR-8674 'transition from solr.tests.mergePolicy to solr.tests.mergePolicyFactory' ticket. So then we are done with SOLR-8621 here I think - what do you think? (The Solr Reference Guide changes still to be done separately and likely directly in Confluence.)
          Hide
          Shai Erera added a comment -

          Christine Poerschke these look good, so +1 to merge to 5x. I agree that we're done w/ this issue here. We can separately take care of SOLR-8674 and SOLR-8668. I enjoyed this collaboration, thank you very much for such a fun and positive experience!

          Show
          Shai Erera added a comment - Christine Poerschke these look good, so +1 to merge to 5x. I agree that we're done w/ this issue here. We can separately take care of SOLR-8674 and SOLR-8668 . I enjoyed this collaboration, thank you very much for such a fun and positive experience!
          Hide
          ASF subversion and git services added a comment -

          Commit b2e83b6788b5b3c9ee6cf13be1cddec7f21014f4 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b2e83b6 ]

          SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method

          Here in SOLR-8621:

          • UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory
          • (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory

          Elsewhere in SOLR-5730:

          • SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Show
          ASF subversion and git services added a comment - Commit b2e83b6788b5b3c9ee6cf13be1cddec7f21014f4 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b2e83b6 ] SOLR-8621 : factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method Here in SOLR-8621 : UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory Elsewhere in SOLR-5730 : SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Hide
          ASF subversion and git services added a comment -

          Commit b2e83b6788b5b3c9ee6cf13be1cddec7f21014f4 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b2e83b6 ]

          SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method

          Here in SOLR-8621:

          • UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory
          • (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory

          Elsewhere in SOLR-5730:

          • SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Show
          ASF subversion and git services added a comment - Commit b2e83b6788b5b3c9ee6cf13be1cddec7f21014f4 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=b2e83b6 ] SOLR-8621 : factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method Here in SOLR-8621 : UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory Elsewhere in SOLR-5730 : SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Hide
          ASF subversion and git services added a comment -

          Commit d76be85a42e58ff906342c1283b3d4608d59df36 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d76be85 ]

          SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for existing (test-framework) RandomForceMergePolicy

          Show
          ASF subversion and git services added a comment - Commit d76be85a42e58ff906342c1283b3d4608d59df36 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d76be85 ] SOLR-8621 : add (test-framework) RandomForceMergePolicyFactory for existing (test-framework) RandomForceMergePolicy
          Hide
          ASF subversion and git services added a comment -

          Commit a267fa729bc62267173760ca5a05e912f7089fa9 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a267fa7 ]

          SOLR-8621: solrconfig.xml in solr/server/solr/configsets and test-files/solr/configsets/bad-mergepolicy now also use <mergePolicyFactory> instead of <mergePolicy>

          Show
          ASF subversion and git services added a comment - Commit a267fa729bc62267173760ca5a05e912f7089fa9 in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=a267fa7 ] SOLR-8621 : solrconfig.xml in solr/server/solr/configsets and test-files/solr/configsets/bad-mergepolicy now also use <mergePolicyFactory> instead of <mergePolicy>
          Hide
          ASF subversion and git services added a comment -

          Commit 6a9d89309595ff668f851c17af4f3f97af7640c1 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6a9d893 ]

          SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method

          Here in SOLR-8621:

          • UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory
          • (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory

          Elsewhere in SOLR-5730:

          • SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Show
          ASF subversion and git services added a comment - Commit 6a9d89309595ff668f851c17af4f3f97af7640c1 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6a9d893 ] SOLR-8621 : factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method Here in SOLR-8621 : UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory Elsewhere in SOLR-5730 : SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Hide
          ASF subversion and git services added a comment -

          Commit 6a9d89309595ff668f851c17af4f3f97af7640c1 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6a9d893 ]

          SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method

          Here in SOLR-8621:

          • UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory
          • (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory

          Elsewhere in SOLR-5730:

          • SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Show
          ASF subversion and git services added a comment - Commit 6a9d89309595ff668f851c17af4f3f97af7640c1 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6a9d893 ] SOLR-8621 : factor out protected abstract WrapperMergePolicyFactory.getMergePolicyInstance method Here in SOLR-8621 : UpgradeIndexMergePolicyFactory extends WrapperMergePolicyFactory (WrapperMergePolicyFactoryTest's) DefaultingWrapperMergePolicyFactory extends WrapperMergePolicyFactory Elsewhere in SOLR-5730 : SortingMergePolicyFactory will extend WrapperMergePolicyFactory
          Hide
          ASF subversion and git services added a comment -

          Commit d49d9dac835ecabd0c5202507cb98d2fdd6d95c5 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d49d9da ]

          SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for existing (test-framework) RandomForceMergePolicy

          Show
          ASF subversion and git services added a comment - Commit d49d9dac835ecabd0c5202507cb98d2fdd6d95c5 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=d49d9da ] SOLR-8621 : add (test-framework) RandomForceMergePolicyFactory for existing (test-framework) RandomForceMergePolicy
          Hide
          ASF subversion and git services added a comment -

          Commit 4c7c1d3c58d2398de4cc651025a32125afe7adbc in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4c7c1d3 ]

          SOLR-8621: solrconfig.xml in solr/server/solr/configsets and test-files/solr/configsets/bad-mergepolicy now also use <mergePolicyFactory> instead of <mergePolicy>

          Show
          ASF subversion and git services added a comment - Commit 4c7c1d3c58d2398de4cc651025a32125afe7adbc in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=4c7c1d3 ] SOLR-8621 : solrconfig.xml in solr/server/solr/configsets and test-files/solr/configsets/bad-mergepolicy now also use <mergePolicyFactory> instead of <mergePolicy>
          Hide
          Christine Poerschke added a comment - - edited

          Shai Erera - likewise, thank you for collaborating on this, I also enjoyed it as a fun and positive experience.

          Cherry-picking to branch_5x and branch_5_5 is complete. Yesterday I also git push origin --delete master-solr-8621 tidied up the working branch.

          Show
          Christine Poerschke added a comment - - edited Shai Erera - likewise, thank you for collaborating on this, I also enjoyed it as a fun and positive experience. Cherry-picking to branch_5x and branch_5_5 is complete. Yesterday I also git push origin --delete master-solr-8621 tidied up the working branch.
          Hide
          Christine Poerschke added a comment -

          This ticket is complete and the changes on master are present in branch_5x and branch_5_5.

          branch_5_5:

          $ git logdog --oneline origin/branch_5_5 | grep "SOLR-8621" | cut -b1-80
          * 4c7c1d3 (origin/branch_5_5, branch_5_5) SOLR-8621: solrconfig.xml in solr/serv
          * d49d9da SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for exis
          * 6a9d893 SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.get
          * 3f06f9a SOLR-8621: WrapperMergePolicyFactory logic tweaks
          * 47055d5 SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in comme
          * 5d10650 SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor
          * 24f4025 SOLR-8621: fix solrconfig.xml under contrib and example
          * 594145f SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test coverage
          * 39fd942 SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now rando
          * b052a8f SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now also ment
          * 67bf26f SOLR-8621: add missing package-info.java
          * eac5a35 SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFactory>
          

          branch_5x:

          $ git logdog --oneline origin/branch_5x | grep "SOLR-8621" | cut -b1-80
          * a267fa7 (origin/branch_5x, branch_5x) SOLR-8621: solrconfig.xml in solr/server
          * d76be85 SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for exis
          * b2e83b6 SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.get
          * bdef478 SOLR-8621: WrapperMergePolicyFactory logic tweaks
          * bbbc90f SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in comme
          * 5d10650 SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor
          * 24f4025 SOLR-8621: fix solrconfig.xml under contrib and example
          * 594145f SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test coverage
          * 39fd942 SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now rando
          * b052a8f SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now also ment
          * 67bf26f SOLR-8621: add missing package-info.java
          * eac5a35 SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFactory>
          

          master:

          $ git logdog --oneline origin/master | grep "SOLR-8621" | cut -b1-80
          * 159ace1 (HEAD -> master, origin/master, origin/HEAD) SOLR-8621: solrconfig.xml
          * b2e4798 SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for exis
          * 360051a SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.get
          * | | | 6b6932e SOLR-8621: WrapperMergePolicyFactory logic tweaks
          * | | | 588e3ff SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in
          * | | | 5d32609 SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor
          * | | | a5acccc SOLR-8621: fix solrconfig.xml under contrib and example
          * | | | b47eeb2 SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test cove
          * | | | e9c9003 SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now
          * | | | 3603760 SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now als
          * | | | 71f4e01 SOLR-8621: add missing package-info.java
          * | | | fe2cf25 SOLR-8621: Fix SolrIndexConfig.toMap() to use mergePolicyFactory
          * | | | fc5b1ac SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFacto
          

          (I have confirmed that the extra fe2cf250796a98ff1791a504d21acb67f0a1c397 commit on master is present in branch_5x and branch_5_5 as part of their initial 'deprecate <mergePolicy> in favor of <mergePolicyFactory>' commits.)

          Show
          Christine Poerschke added a comment - This ticket is complete and the changes on master are present in branch_5x and branch_5_5. branch_5_5: $ git logdog --oneline origin/branch_5_5 | grep "SOLR-8621" | cut -b1-80 * 4c7c1d3 (origin/branch_5_5, branch_5_5) SOLR-8621: solrconfig.xml in solr/serv * d49d9da SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for exis * 6a9d893 SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.get * 3f06f9a SOLR-8621: WrapperMergePolicyFactory logic tweaks * 47055d5 SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in comme * 5d10650 SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor * 24f4025 SOLR-8621: fix solrconfig.xml under contrib and example * 594145f SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test coverage * 39fd942 SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now rando * b052a8f SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now also ment * 67bf26f SOLR-8621: add missing package -info.java * eac5a35 SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFactory> branch_5x: $ git logdog --oneline origin/branch_5x | grep "SOLR-8621" | cut -b1-80 * a267fa7 (origin/branch_5x, branch_5x) SOLR-8621: solrconfig.xml in solr/server * d76be85 SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for exis * b2e83b6 SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.get * bdef478 SOLR-8621: WrapperMergePolicyFactory logic tweaks * bbbc90f SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in comme * 5d10650 SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor * 24f4025 SOLR-8621: fix solrconfig.xml under contrib and example * 594145f SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test coverage * 39fd942 SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now rando * b052a8f SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now also ment * 67bf26f SOLR-8621: add missing package -info.java * eac5a35 SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFactory> master: $ git logdog --oneline origin/master | grep "SOLR-8621" | cut -b1-80 * 159ace1 (HEAD -> master, origin/master, origin/HEAD) SOLR-8621: solrconfig.xml * b2e4798 SOLR-8621: add (test-framework) RandomForceMergePolicyFactory for exis * 360051a SOLR-8621: factor out protected abstract WrapperMergePolicyFactory.get * | | | 6b6932e SOLR-8621: WrapperMergePolicyFactory logic tweaks * | | | 588e3ff SOLR-8621: fix mergePolicyFacory vs. mergePolicyFactory typos in * | | | 5d32609 SOLR-8621: add IndexSchema arg to MergePolicyFactory constructor * | | | a5acccc SOLR-8621: fix solrconfig.xml under contrib and example * | | | b47eeb2 SOLR-8621: more <mergePolicy> vs. <mergePolicyFactory> test cove * | | | e9c9003 SOLR-8621: TestMergePolicyConfig.testTieredMergePolicyConfig now * | | | 3603760 SOLR-8621: solr/CHANGES.txt 'Upgrading from 5.4' section now als * | | | 71f4e01 SOLR-8621: add missing package -info.java * | | | fe2cf25 SOLR-8621: Fix SolrIndexConfig.toMap() to use mergePolicyFactory * | | | fc5b1ac SOLR-8621: deprecate <mergePolicy> in favor of <mergePolicyFacto (I have confirmed that the extra fe2cf250796a98ff1791a504d21acb67f0a1c397 commit on master is present in branch_5x and branch_5_5 as part of their initial 'deprecate <mergePolicy> in favor of <mergePolicyFactory>' commits.)
          Hide
          Christine Poerschke added a comment -

          Err, oops, having done that master/branch_5x/branch_5_5 consistency check above and looked at the fe2cf250796a98ff1791a504d21acb67f0a1c397 commit again, I think we need one more little tweak:

               if (mergePolicyInfo != null) {
                 m.put("mergePolicy", mergePolicyInfo.toMap());
               } else if (mergePolicyFactoryInfo != null) {
          -      m.put("mergePolicy", mergePolicyFactoryInfo.toMap());
          +      m.put("mergePolicyFactory", mergePolicyFactoryInfo.toMap());
               }
          

          with likely an accompanying test code change.

          Show
          Christine Poerschke added a comment - Err, oops, having done that master/branch_5x/branch_5_5 consistency check above and looked at the fe2cf250796a98ff1791a504d21acb67f0a1c397 commit again, I think we need one more little tweak: if (mergePolicyInfo != null ) { m.put( "mergePolicy" , mergePolicyInfo.toMap()); } else if (mergePolicyFactoryInfo != null ) { - m.put( "mergePolicy" , mergePolicyFactoryInfo.toMap()); + m.put( "mergePolicyFactory" , mergePolicyFactoryInfo.toMap()); } with likely an accompanying test code change.
          Hide
          ASF subversion and git services added a comment -

          Commit 77558a649fb4412e42c0a4d1dd0007f0c539b8b2 in lucene-solr's branch refs/heads/master from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=77558a6 ]

          SOLR-8621: SolrIndexConfig.toMap() fix to distinguish mergePolicyInfo and mergePolicyFactoryInfo, associated SolrIndexConfigTest and TestConfig tweaks.

          Show
          ASF subversion and git services added a comment - Commit 77558a649fb4412e42c0a4d1dd0007f0c539b8b2 in lucene-solr's branch refs/heads/master from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=77558a6 ] SOLR-8621 : SolrIndexConfig.toMap() fix to distinguish mergePolicyInfo and mergePolicyFactoryInfo, associated SolrIndexConfigTest and TestConfig tweaks.
          Hide
          ASF subversion and git services added a comment -

          Commit 8488cc7ba2188a0d8212af3b876090541cc9fb0a in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8488cc7 ]

          SOLR-8621: SolrIndexConfig.toMap() fix to distinguish mergePolicyInfo and mergePolicyFactoryInfo, associated SolrIndexConfigTest and TestConfig tweaks.

          Show
          ASF subversion and git services added a comment - Commit 8488cc7ba2188a0d8212af3b876090541cc9fb0a in lucene-solr's branch refs/heads/branch_5x from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8488cc7 ] SOLR-8621 : SolrIndexConfig.toMap() fix to distinguish mergePolicyInfo and mergePolicyFactoryInfo, associated SolrIndexConfigTest and TestConfig tweaks.
          Hide
          ASF subversion and git services added a comment -

          Commit 8c60352182f721f897b88f6111ff5c28cd66bc65 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8c60352 ]

          SOLR-8621: SolrIndexConfig.toMap() fix to distinguish mergePolicyInfo and mergePolicyFactoryInfo, associated SolrIndexConfigTest and TestConfig tweaks.

          Show
          ASF subversion and git services added a comment - Commit 8c60352182f721f897b88f6111ff5c28cd66bc65 in lucene-solr's branch refs/heads/branch_5_5 from Christine Poerschke [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=8c60352 ] SOLR-8621 : SolrIndexConfig.toMap() fix to distinguish mergePolicyInfo and mergePolicyFactoryInfo, associated SolrIndexConfigTest and TestConfig tweaks.
          Hide
          Christine Poerschke added a comment -

          77558a649fb4412e42c0a4d1dd0007f0c539b8b2 is master commit for this.

          Show
          Christine Poerschke added a comment - 77558a649fb4412e42c0a4d1dd0007f0c539b8b2 is master commit for this.
          Hide
          Hoss Man added a comment -
          Show
          Hoss Man added a comment - Christine Poerschke & Shai Erera ... can you guys please spot check the edits i made to the ref guide for this issue... https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=32604214&selectedPageVersions=31&selectedPageVersions=32
          Hide
          Jack Krupansky added a comment -

          Shouldn't the index config reference page still list <mergePolicy> but with a "Deprecated" notice? Ditto for <mergeFactor>.

          The Upgrading Solr ref page does give an example of how to migrate from MP to MPF (and for MF) - it would be nice to link to that from a deprecated notice on the index config page.

          See:
          https://cwiki.apache.org/confluence/display/solr/IndexConfig+in+SolrConfig
          https://cwiki.apache.org/confluence/display/solr/Upgrading+Solr

          Show
          Jack Krupansky added a comment - Shouldn't the index config reference page still list <mergePolicy> but with a "Deprecated" notice? Ditto for <mergeFactor>. The Upgrading Solr ref page does give an example of how to migrate from MP to MPF (and for MF) - it would be nice to link to that from a deprecated notice on the index config page. See: https://cwiki.apache.org/confluence/display/solr/IndexConfig+in+SolrConfig https://cwiki.apache.org/confluence/display/solr/Upgrading+Solr
          Hide
          Hoss Man added a comment -

          Shouldn't the index config reference page still list <mergePolicy> but with a "Deprecated" notice?

          No.

          The ref guide is not a living document, there are specific versions for each release that show the current way of doing things (not the old way). When users lookup how to do something we don't want to confuse them by telling them how to do something that is no longer supported. If you want to know how to edit older configs, look at older versions of the ref guide.

          Show
          Hoss Man added a comment - Shouldn't the index config reference page still list <mergePolicy> but with a "Deprecated" notice? No. The ref guide is not a living document, there are specific versions for each release that show the current way of doing things (not the old way). When users lookup how to do something we don't want to confuse them by telling them how to do something that is no longer supported. If you want to know how to edit older configs, look at older versions of the ref guide.
          Show
          Christine Poerschke added a comment - ... spot check the edits i made to the ref guide ... https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=32604214&selectedPageVersions=31&selectedPageVersions=32 edit looks good to me. https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=32604214&selectedPageVersions=35&selectedPageVersions=37 is the 'custom merge policies' paragraph i added - what do you think?
          Hide
          Shai Erera added a comment -

          I am not sure how to comment on those pages, so I'll comment here:

          <mergePolicyFactory class="org.apache.lucene.index.TieredMergePolicy">

          Shouldn't the class be TieredMergePolicyFactory?

          This process can continue indefinitely

          Well, not indefinitely . More like "The process can continue until there are no more mergeFactor segments to merge of same size".

          and the maxMergeAtOnce setting determines how many segments should be included in the merge

          Perhaps "... should be included in each merge"? Cause if segmentsPerTier is 30 and maxMergeAtOnce is 10, there will be 3 merges.

          It also can also result

          One extra 'also' here.

          class="MyCustomMergePolicyFactory"

          Should we write class="full.package.MyCustomMergePolicyFactory"? It's not critical but I want to emphasize that one cannot just give the class name here, but needs to FQCN.

          org.apache.solr.index.TieredMergePolicyFactory

          If solr.TieredMergePolicyFactory works too, let's write that? That way, if the factory changes packages, we won't need to update the guide.

          Show
          Shai Erera added a comment - I am not sure how to comment on those pages, so I'll comment here: <mergePolicyFactory class="org.apache.lucene.index.TieredMergePolicy"> Shouldn't the class be TieredMergePolicyFactory ? This process can continue indefinitely Well, not indefinitely . More like "The process can continue until there are no more mergeFactor segments to merge of same size". and the maxMergeAtOnce setting determines how many segments should be included in the merge Perhaps "... should be included in each merge"? Cause if segmentsPerTier is 30 and maxMergeAtOnce is 10, there will be 3 merges. It also can also result One extra 'also' here. class="MyCustomMergePolicyFactory" Should we write class="full.package.MyCustomMergePolicyFactory" ? It's not critical but I want to emphasize that one cannot just give the class name here, but needs to FQCN. org.apache.solr.index.TieredMergePolicyFactory If solr.TieredMergePolicyFactory works too, let's write that? That way, if the factory changes packages, we won't need to update the guide.
          Hide
          Christine Poerschke added a comment -

          Thanks Shai Erera!

          This is the latest end-to-end combined edit: https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=32604214&selectedPageVersions=31&selectedPageVersions=38

          Looks like mergeScheduler and mergedSegmentWarmer use org.apache.lucene.index. - so perhaps for consistency mergePolicyFactory should maintain that style even if solr.TieredMergePolicyFactory were to also work (haven't tried it).

          Show
          Christine Poerschke added a comment - Thanks Shai Erera ! This is the latest end-to-end combined edit: https://cwiki.apache.org/confluence/pages/diffpagesbyversion.action?pageId=32604214&selectedPageVersions=31&selectedPageVersions=38 Looks like mergeScheduler and mergedSegmentWarmer use org.apache.lucene.index. - so perhaps for consistency mergePolicyFactory should maintain that style even if solr.TieredMergePolicyFactory were to also work (haven't tried it).
          Hide
          Shai Erera added a comment -

          Looks good Christine Poerschke! And yes, let's be consistent and keep the full package name, as the other examples.

          Show
          Shai Erera added a comment - Looks good Christine Poerschke ! And yes, let's be consistent and keep the full package name, as the other examples.
          Hide
          Henrik added a comment -

          Hi Christine Poerschke and others,

          I couldn't find an upgrade document for mergeFactor, so I'll just ask here. This is my current config:

          <indexConfig>
                  <mergeFactor>6</mergeFactor>
                  <maxIndexingThreads>8</maxIndexingThreads>
          </indexConfig>

          If want the exact same behaviour as I currently have, how should it look in Solr6? I can't find mention of maxIndexingThreads in https://cwiki.apache.org/confluence/display/solr/IndexConfig+in+SolrConfig and I haven't figured out if the "old" policy is TieredMergePolicyFactory or
          LogMergePolicy.

          Thanks.

          Show
          Henrik added a comment - Hi Christine Poerschke and others, I couldn't find an upgrade document for mergeFactor , so I'll just ask here. This is my current config: <indexConfig> <mergeFactor>6</mergeFactor> <maxIndexingThreads>8</maxIndexingThreads> </indexConfig> If want the exact same behaviour as I currently have, how should it look in Solr6? I can't find mention of maxIndexingThreads in https://cwiki.apache.org/confluence/display/solr/IndexConfig+in+SolrConfig and I haven't figured out if the "old" policy is TieredMergePolicyFactory or LogMergePolicy . Thanks.
          Hide
          Henrik added a comment -

          I also have this, by the way:

              <mergeScheduler class="org.apache.lucene.index.ConcurrentMergeScheduler">
                  <int name="maxMergeCount">5</int>
              </mergeScheduler>
          Show
          Henrik added a comment - I also have this, by the way: <mergeScheduler class= "org.apache.lucene.index.ConcurrentMergeScheduler" > < int name= "maxMergeCount" >5</ int > </mergeScheduler>
          Hide
          Henrik added a comment -

          Never mind, I found the answer in https://archive.apache.org/dist/lucene/solr/ref-guide/apache-solr-ref-guide-5.4.pdf in "Merging Index Segments".

          Show
          Henrik added a comment - Never mind, I found the answer in https://archive.apache.org/dist/lucene/solr/ref-guide/apache-solr-ref-guide-5.4.pdf in "Merging Index Segments".

            People

            • Assignee:
              Christine Poerschke
              Reporter:
              Christine Poerschke
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development