Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.4
    • Component/s: None
    • Labels:
      None

      Description

      MergePolicy and MergeScheduler require property injection. We'll allow these and probably other cases in this patch using Java reflection.

      1. SOLR-1447.patch
        10 kB
        Noble Paul
      2. SOLR-1447.patch
        28 kB
        Jason Rutherglen
      3. SOLR-1447.patch
        47 kB
        Noble Paul
      4. SOLR-1447.patch
        48 kB
        Noble Paul
      5. SOLR-1447.patch
        49 kB
        Noble Paul
      6. SOLR-1447.patch
        19 kB
        Jason Rutherglen
      7. SOLR-1447-indexDefault-test.patch
        19 kB
        Shalin Shekhar Mangar
      8. SOLR-1447-indexDefault-test.patch
        19 kB
        Shalin Shekhar Mangar

        Issue Links

          Activity

          Hide
          Noble Paul added a comment -

          +1 .

          Show
          Noble Paul added a comment - +1 .
          Hide
          Noble Paul added a comment -

          This is not the complete patch . only code changes .I guess we should change it in 1.4 itself

          Show
          Noble Paul added a comment - This is not the complete patch . only code changes .I guess we should change it in 1.4 itself
          Hide
          Noble Paul added a comment -

          Let us see if we can clean this up in this release itself ?

          Show
          Noble Paul added a comment - Let us see if we can clean this up in this release itself ?
          Hide
          Noble Paul added a comment - - edited

          This would mean that the configuration of mergePolicy and mergeScheduler become as follows

          <mergePolicy class="org.apache.lucene.index.LogByteSizeMergePolicy"/>
          <mergeSceduler class="org.apache.lucene.index.ConcurrentMergeScheduler" />
          

          any setters of the class can be invoked on these classes by keeping attributes

          Show
          Noble Paul added a comment - - edited This would mean that the configuration of mergePolicy and mergeScheduler become as follows <mergePolicy class= "org.apache.lucene.index.LogByteSizeMergePolicy" /> <mergeSceduler class= "org.apache.lucene.index.ConcurrentMergeScheduler" /> any setters of the class can be invoked on these classes by keeping attributes
          Hide
          Jason Rutherglen added a comment -

          Noble, looks good, can you include a test case?

          Show
          Jason Rutherglen added a comment - Noble, looks good, can you include a test case?
          Hide
          Jason Rutherglen added a comment -

          Actually if we're going to change the solrconfig xml schema, I think it should look like:

          <mergePolicy class="org.apache.lucene.index.LogByteSizeMergePolicy">
            <double name="maxMergeMB">64.0</double>
          </mergePolicy>
          <mergeSceduler class="org.apache.lucene.index.ConcurrentMergeScheduler">
            <int name="maxThreadCount">3</int>
          </mergeScheduler>
          

          Attributes are limiting. This way we can add more complex objects in the future if needed.

          Show
          Jason Rutherglen added a comment - Actually if we're going to change the solrconfig xml schema, I think it should look like: <mergePolicy class= "org.apache.lucene.index.LogByteSizeMergePolicy" > < double name= "maxMergeMB" >64.0</ double > </mergePolicy> <mergeSceduler class= "org.apache.lucene.index.ConcurrentMergeScheduler" > < int name= "maxThreadCount" >3</ int > </mergeScheduler> Attributes are limiting. This way we can add more complex objects in the future if needed.
          Hide
          Noble Paul added a comment -

          attributes are limiting. This way we can add more complex objects in the future if needed.

          We are not limiting to attributes. If the Object is a NamedListInitializedPlugin it can use the sub attributes also. If we use external classes we cannot dictate that class to implement a NamedListInitializedPlugin . So let us have both

          Show
          Noble Paul added a comment - attributes are limiting. This way we can add more complex objects in the future if needed. We are not limiting to attributes. If the Object is a NamedListInitializedPlugin it can use the sub attributes also. If we use external classes we cannot dictate that class to implement a NamedListInitializedPlugin . So let us have both
          Hide
          Jason Rutherglen added a comment -

          Here's a patch with a test case that implements init args.

          If we stick with one system, it's less to document and debug later.

          Show
          Jason Rutherglen added a comment - Here's a patch with a test case that implements init args. If we stick with one system, it's less to document and debug later.
          Hide
          Noble Paul added a comment -

          Jason .I am fine with this. I plan to commit this shortly

          Show
          Noble Paul added a comment - Jason .I am fine with this. I plan to commit this shortly
          Hide
          Noble Paul added a comment -

          all the conf files needed to be modified for this

          Show
          Noble Paul added a comment - all the conf files needed to be modified for this
          Hide
          Noble Paul added a comment -

          a test was failing

          Show
          Noble Paul added a comment - a test was failing
          Hide
          Jason Rutherglen added a comment -

          Noble, looks good, and all tests pass. I think it's tricky to commit this as is, because there's no back compat? Maybe we can allow:

          <mergePolicy>org.apache.lucene.index.LogByteSizeMergePolicy</mergePolicy>
          <mergeScheduler>org.apache.lucene.index.ConcurrentMergeScheduler</mergeScheduler>
          

          or

          <mergePolicy14 class="org.apache.lucene.index.LogByteSizeMergePolicy">
            <double name="maxMergeMB">64.0</double>
          </mergePolicy14>
          <mergeSceduler14 class="org.apache.lucene.index.ConcurrentMergeScheduler">
            <int name="maxThreadCount">3</int>
          </mergeScheduler14>
          

          It's ugly but more clear and this way users' installations won't immediately break on deployment.

          Show
          Jason Rutherglen added a comment - Noble, looks good, and all tests pass. I think it's tricky to commit this as is, because there's no back compat? Maybe we can allow: <mergePolicy>org.apache.lucene.index.LogByteSizeMergePolicy</mergePolicy> <mergeScheduler>org.apache.lucene.index.ConcurrentMergeScheduler</mergeScheduler> or <mergePolicy14 class= "org.apache.lucene.index.LogByteSizeMergePolicy" > < double name= "maxMergeMB" >64.0</ double > </mergePolicy14> <mergeSceduler14 class= "org.apache.lucene.index.ConcurrentMergeScheduler" > < int name= "maxThreadCount" >3</ int > </mergeScheduler14> It's ugly but more clear and this way users' installations won't immediately break on deployment.
          Hide
          Noble Paul added a comment -

          yes, there is no back-compat. but changing the tag-name is not an option

          The ideal fix would be to support both syntaxes.I guess it is still possible.

          Show
          Noble Paul added a comment - yes, there is no back-compat. but changing the tag-name is not an option The ideal fix would be to support both syntaxes.I guess it is still possible.
          Hide
          Jason Rutherglen added a comment -

          Why is changing the tag name not an option?

          Show
          Jason Rutherglen added a comment - Why is changing the tag name not an option?
          Hide
          Noble Paul added a comment -

          the back-compat thing is a temporary thing. After we remove it we would love to keep the name as mergePolicy or mergeScheduler itself. There is no reason we should have this arbitrary number at the end.

          Show
          Noble Paul added a comment - the back-compat thing is a temporary thing. After we remove it we would love to keep the name as mergePolicy or mergeScheduler itself. There is no reason we should have this arbitrary number at the end.
          Hide
          Noble Paul added a comment -

          supports both syntax. logs out a warning if the old syntax is used

          Show
          Noble Paul added a comment - supports both syntax. logs out a warning if the old syntax is used
          Hide
          Noble Paul added a comment -

          committed : r817650
          Thanks Jason

          Show
          Noble Paul added a comment - committed : r817650 Thanks Jason
          Hide
          Noble Paul added a comment -

          this turned out to be a lot bigger

          Show
          Noble Paul added a comment - this turned out to be a lot bigger
          Hide
          Jason Rutherglen added a comment -

          I think we need a legacy test?

          Show
          Jason Rutherglen added a comment - I think we need a legacy test?
          Hide
          Jason Rutherglen added a comment -

          I added a legacy test, and fixed a small NPE bug that happens via SolrPlugUtils

          Show
          Jason Rutherglen added a comment - I added a legacy test, and fixed a small NPE bug that happens via SolrPlugUtils
          Hide
          Noble Paul added a comment -

          committed r817976

          thanks Jason

          Show
          Noble Paul added a comment - committed r817976 thanks Jason
          Hide
          Shalin Shekhar Mangar added a comment -

          Re-opening because the mergeScheduler and mergePolicy specified in the indexDefaults section is not honored.

          Show
          Shalin Shekhar Mangar added a comment - Re-opening because the mergeScheduler and mergePolicy specified in the indexDefaults section is not honored.
          Hide
          Shalin Shekhar Mangar added a comment -

          Here's a test case which fails.

          Still trying to understand and debug.

          Show
          Shalin Shekhar Mangar added a comment - Here's a test case which fails. Still trying to understand and debug.
          Hide
          Shalin Shekhar Mangar added a comment -

          Fix which passes the test case posted earlier. I'll commit this shortly.

          Show
          Shalin Shekhar Mangar added a comment - Fix which passes the test case posted earlier. I'll commit this shortly.
          Hide
          Shalin Shekhar Mangar added a comment -

          Committed revision 819401.

          Show
          Shalin Shekhar Mangar added a comment - Committed revision 819401.
          Hide
          Grant Ingersoll added a comment -

          Bulk close for Solr 1.4

          Show
          Grant Ingersoll added a comment - Bulk close for Solr 1.4

            People

            • Assignee:
              Noble Paul
              Reporter:
              Jason Rutherglen
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 48h
                48h
                Remaining:
                Remaining Estimate - 48h
                48h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development