Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, trunk
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      We currently have two MergePolicy implementations that are wrappers around another MP: SortingMergePolicy and UpgradeIndexMergePolicy. A MergePolicyWrapper will simplify building additional such wrapping MPs by delegating all method calls to the wrapped instance, and allowing implementations to override only what they need.

      Also, this issue removes the final modifier from MP public methods so that they can be delegated properly. See LUCENE-7008 for a test failure that uncovered this issue.

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          +1

          Show
          Uwe Schindler added a comment - +1
          Hide
          Shai Erera added a comment -

          Patch adds MergePolicyWrapper and modifies (quite a few) existing MPs to extend it.

          Show
          Shai Erera added a comment - Patch adds MergePolicyWrapper and modifies (quite a few) existing MPs to extend it.
          Hide
          Uwe Schindler added a comment -

          Why this in NoMergePolicy?

          +  @Override
          +  public double getNoCFSRatio() {
          +    return super.getNoCFSRatio();
          +  }
          +  
          +  @Override
          +  public void setMaxCFSSegmentSizeMB(double v) {
          +    super.setMaxCFSSegmentSizeMB(v);
          +  }
          +  
          +  @Override
          +  public void setNoCFSRatio(double noCFSRatio) {
          +    super.setNoCFSRatio(noCFSRatio);
          +  }
          
          Show
          Uwe Schindler added a comment - Why this in NoMergePolicy? + @Override + public double getNoCFSRatio() { + return super.getNoCFSRatio(); + } + + @Override + public void setMaxCFSSegmentSizeMB(double v) { + super.setMaxCFSSegmentSizeMB(v); + } + + @Override + public void setNoCFSRatio(double noCFSRatio) { + super.setNoCFSRatio(noCFSRatio); + }
          Hide
          Shai Erera added a comment -

          Why this in NoMergePolicy?

          Because TestNoMergePolicy asserts that all MP methods are overridden by NoMergePolicy. Since those methods have no actual effect on NoMP I've decided to delegate them to super.

          Show
          Shai Erera added a comment - Why this in NoMergePolicy? Because TestNoMergePolicy asserts that all MP methods are overridden by NoMergePolicy . Since those methods have no actual effect on NoMP I've decided to delegate them to super.
          Hide
          Uwe Schindler added a comment -

          Sorry I still don't get it! In your patch, TestMergePolicyWrapper is the only place that checks if all methods are overriden, but NoMergePolicy extends MergePolicy and there is no TestNoMergePolicy. Could it be that your patch is missing a file?

          In any case I dont understand why this test should exist at all for NoMergePolicy!?!

          Show
          Uwe Schindler added a comment - Sorry I still don't get it! In your patch, TestMergePolicyWrapper is the only place that checks if all methods are overriden, but NoMergePolicy extends MergePolicy and there is no TestNoMergePolicy. Could it be that your patch is missing a file? In any case I dont understand why this test should exist at all for NoMergePolicy!?!
          Hide
          Shai Erera added a comment -

          How can you not find TestNoMergePolicy? It's been existed in the code since NoMergePolicy was added. Here's the relevant test that I was talking about:

            @Test
            public void testMethodsOverridden() throws Exception {
              // Ensures that all methods of MergePolicy are overridden. That's important
              // to ensure that NoMergePolicy overrides everything, so that no unexpected
              // behavior/error occurs
              for (Method m : NoMergePolicy.class.getMethods()) {
                // getDeclaredMethods() returns just those methods that are declared on
                // NoMergePolicy. getMethods() returns those that are visible in that
                // context, including ones from Object. So just filter out Object. If in
                // the future MergePolicy will extend a different class than Object, this
                // will need to change.
                if (m.getName().equals("clone")) {
                  continue;
                }
                if (m.getDeclaringClass() != Object.class && !Modifier.isFinal(m.getModifiers())) {
                  assertTrue(m + " is not overridden ! ", m.getDeclaringClass() == NoMergePolicy.class);
                }
              }
            }
          

          I don't mind removing this particular test, but it's there to ensure that if MP adds more methods in the future that NoMP should handle, we catch that.

          Show
          Shai Erera added a comment - How can you not find TestNoMergePolicy ? It's been existed in the code since NoMergePolicy was added. Here's the relevant test that I was talking about: @Test public void testMethodsOverridden() throws Exception { // Ensures that all methods of MergePolicy are overridden. That's important // to ensure that NoMergePolicy overrides everything, so that no unexpected // behavior/error occurs for (Method m : NoMergePolicy.class.getMethods()) { // getDeclaredMethods() returns just those methods that are declared on // NoMergePolicy. getMethods() returns those that are visible in that // context, including ones from Object . So just filter out Object . If in // the future MergePolicy will extend a different class than Object , this // will need to change. if (m.getName().equals( "clone" )) { continue ; } if (m.getDeclaringClass() != Object .class && !Modifier.isFinal(m.getModifiers())) { assertTrue(m + " is not overridden ! " , m.getDeclaringClass() == NoMergePolicy.class); } } } I don't mind removing this particular test, but it's there to ensure that if MP adds more methods in the future that NoMP should handle, we catch that.
          Hide
          Shai Erera added a comment -

          Uwe Schindler do you still have concerns about it? I agree it looks silly that NoMP overrides methods just to call super(). So we have few options:

          • I keep the overrides, but don't do anything in the setters and return 0 in the getters.
          • We drop these overrides and either tweak the test, or remove it.

          If we need to change anything, I'd prefer to do the former. What do you think?

          Show
          Shai Erera added a comment - Uwe Schindler do you still have concerns about it? I agree it looks silly that NoMP overrides methods just to call super(). So we have few options: I keep the overrides, but don't do anything in the setters and return 0 in the getters. We drop these overrides and either tweak the test, or remove it. If we need to change anything, I'd prefer to do the former. What do you think?
          Hide
          Uwe Schindler added a comment -

          All fine. I was looking for a test in your file. I was not aware that there was already one.

          Show
          Uwe Schindler added a comment - All fine. I was looking for a test in your file. I was not aware that there was already one.
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-7010: add MergePolicyWrapper

          Show
          ASF subversion and git services added a comment - Commit 30455f728b304fb1b434df73b4f84d607e6941ce in lucene-solr's branch refs/heads/master from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=30455f7 ] LUCENE-7010 : add MergePolicyWrapper
          Hide
          ASF subversion and git services added a comment -

          Commit 43508831b1eadd6610909faac4accd8007753bbb 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=4350883 ]

          LUCENE-7010: add MergePolicyWrapper

          Show
          ASF subversion and git services added a comment - Commit 43508831b1eadd6610909faac4accd8007753bbb 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=4350883 ] LUCENE-7010 : add MergePolicyWrapper
          Hide
          Shai Erera added a comment -

          Thanks Uwe Schindler, committed to trunk and 5x.

          Show
          Shai Erera added a comment - Thanks Uwe Schindler , committed to trunk and 5x.
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-7010: add @param to ctor javadocs

          Show
          ASF subversion and git services added a comment - Commit 0c4598d5f4629b515906b31b5977b11882bc944b in lucene-solr's branch refs/heads/master from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0c4598d ] LUCENE-7010 : add @param to ctor javadocs
          Hide
          ASF subversion and git services added a comment -

          Commit 28ed648fbf92fb0c120f0c84d8c7de14772ca490 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=28ed648 ]

          LUCENE-7010: add @param to ctor javadocs

          Show
          ASF subversion and git services added a comment - Commit 28ed648fbf92fb0c120f0c84d8c7de14772ca490 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=28ed648 ] LUCENE-7010 : add @param to ctor javadocs
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-7010: document protected field

          Show
          ASF subversion and git services added a comment - Commit e6d629d527fe316e0840e1d2fe5ab1d447ce0460 in lucene-solr's branch refs/heads/master from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e6d629d ] LUCENE-7010 : document protected field
          Hide
          ASF subversion and git services added a comment -

          Commit 5919680482b63901e3c461f4e6369524290042a3 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=5919680 ]

          LUCENE-7010: document protected field

          Show
          ASF subversion and git services added a comment - Commit 5919680482b63901e3c461f4e6369524290042a3 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=5919680 ] LUCENE-7010 : document protected field
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-7010: add MergePolicyWrapper

          Show
          ASF subversion and git services added a comment - Commit 30455f728b304fb1b434df73b4f84d607e6941ce in lucene-solr's branch refs/heads/lucene-6997 from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=30455f7 ] LUCENE-7010 : add MergePolicyWrapper
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-7010: add @param to ctor javadocs

          Show
          ASF subversion and git services added a comment - Commit 0c4598d5f4629b515906b31b5977b11882bc944b in lucene-solr's branch refs/heads/lucene-6997 from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=0c4598d ] LUCENE-7010 : add @param to ctor javadocs
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-7010: document protected field

          Show
          ASF subversion and git services added a comment - Commit e6d629d527fe316e0840e1d2fe5ab1d447ce0460 in lucene-solr's branch refs/heads/lucene-6997 from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e6d629d ] LUCENE-7010 : document protected field
          Hide
          ASF subversion and git services added a comment -

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

          LUCENE-7010: document protected field

          Show
          ASF subversion and git services added a comment - Commit e6d629d527fe316e0840e1d2fe5ab1d447ce0460 in lucene-solr's branch refs/heads/lucene-6835 from Shai Erera [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=e6d629d ] LUCENE-7010 : document protected field

            People

            • Assignee:
              Shai Erera
              Reporter:
              Shai Erera
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development