Lucene - Core
  1. Lucene - Core
  2. LUCENE-5080

CMS setters cannot work unless you setMaxMergeCount before you setMaxThreadCount

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, Trunk
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

        public void setMaxThreadCount(int count) {
          ...
          if (count > maxMergeCount) {
            throw new IllegalArgumentException("count should be <= maxMergeCount (= " + maxMergeCount + ")");
          }
      

      but:

         public void setMaxMergeCount(int count) {
          ...
          if (count < maxThreadCount) {
            throw new IllegalArgumentException("count should be >= maxThreadCount (= " + maxThreadCount + ")");
          }
      

      So you must call them in a magical order. I think we should nuke these setters and just have a CMS(int,int)

      1. LUCENE-5080.patch
        9 kB
        Robert Muir
      2. LUCENE-5080.patch
        11 kB
        Robert Muir

        Activity

        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Shai Erera added a comment -

        The only concern I have is that someone may want to dynamically set that based on some overall application workload. Since MS is on LiveIWC, it's fine cause you can reset to a new CMS instance. But in that case what will happen to the other CMS which loops until IW has no more merges? Is that ok?

        Maybe instead we can have CMS.reset(int,int) to avoid that case?

        Show
        Shai Erera added a comment - The only concern I have is that someone may want to dynamically set that based on some overall application workload. Since MS is on LiveIWC, it's fine cause you can reset to a new CMS instance. But in that case what will happen to the other CMS which loops until IW has no more merges? Is that ok? Maybe instead we can have CMS.reset(int,int) to avoid that case?
        Hide
        Robert Muir added a comment -

        Maybe instead we can have CMS.reset(int,int) to avoid that case?

        Fine by me. if we decide we dont like it later, we can just remove it.

        Show
        Robert Muir added a comment - Maybe instead we can have CMS.reset(int,int) to avoid that case? Fine by me. if we decide we dont like it later, we can just remove it.
        Hide
        Robert Muir added a comment -

        here's my patch. To address Shai's concern, i just combined the two setters into one and didnt do any ctors (that would sorta be redundant with the combined-setter, and i dont like the name reset).

        Additionally i made the defaults public constants.

        Show
        Robert Muir added a comment - here's my patch. To address Shai's concern, i just combined the two setters into one and didnt do any ctors (that would sorta be redundant with the combined-setter, and i dont like the name reset). Additionally i made the defaults public constants.
        Hide
        Robert Muir added a comment -

        updated patch: some solr tests depended on these previous setters with reflection.

        Show
        Robert Muir added a comment - updated patch: some solr tests depended on these previous setters with reflection.
        Hide
        Robert Muir added a comment -

        I committed the new setter. If someone is unhappy about the name, i won't be offended, we can change it.

        At least jenkins will have a nice weekend in the meantime.

        Show
        Robert Muir added a comment - I committed the new setter. If someone is unhappy about the name, i won't be offended, we can change it. At least jenkins will have a nice weekend in the meantime.
        Hide
        Shai Erera added a comment -

        +1 looks good. Thanks Rob!

        Show
        Shai Erera added a comment - +1 looks good. Thanks Rob!
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development