Details

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

      Description

      I think all methods but getPostingsFormatForField should be made final so that users can't create a Codec that redefines any of the formats of Lucene40 by subclassing (since the codec name can't be overriden by subclassing, Lucene will fail at loading segments that use such codecs).

      1. LUCENE-4391.patch
        26 kB
        Adrien Grand
      2. LUCENE-4391.patch
        26 kB
        Adrien Grand

        Activity

        Hide
        Uwe Schindler added a comment -

        +1, I was thinking aout a similar thing, too! If somebody wants to create another codec, reusing some Lucene40 stuff, he should subclass Codec directly and delegate the needed things to Codec.forName("Lucene40").fooBar()

        The same should be done for Lucene40 PostingsFormat!

        Show
        Uwe Schindler added a comment - +1, I was thinking aout a similar thing, too! If somebody wants to create another codec, reusing some Lucene40 stuff, he should subclass Codec directly and delegate the needed things to Codec.forName("Lucene40").fooBar() The same should be done for Lucene40 PostingsFormat!
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Robert Muir added a comment -

        Unassigned issues -> 4.1

        Show
        Robert Muir added a comment - Unassigned issues -> 4.1
        Hide
        Adrien Grand added a comment -

        New patch:

        • makes codecs and postings formats methods final whenever it may lead to broken subclassing,
        • adds a new ForwardingCodec class that makes it easy to build a codec by reusing some functionality of another codec.
        Show
        Adrien Grand added a comment - New patch: makes codecs and postings formats methods final whenever it may lead to broken subclassing, adds a new ForwardingCodec class that makes it easy to build a codec by reusing some functionality of another codec.
        Hide
        Robert Muir added a comment -

        Instead of ForwardingCodec having an abstract delegate(), can we maybe have it look like this:

        FilterCodec(String name, Codec in) {
        ...
        

        (Suggesting FilterCodec to be consistent with FilterReader, FilterTerms, ....)

        Show
        Robert Muir added a comment - Instead of ForwardingCodec having an abstract delegate(), can we maybe have it look like this: FilterCodec( String name, Codec in) { ... (Suggesting FilterCodec to be consistent with FilterReader, FilterTerms, ....)
        Hide
        Adrien Grand added a comment -

        The reason I used a delegate() method is that setting the delegate in the constructor seems to generate cyclic dependencies when used in combination with Codec.forName. (For example, if I use super("Appending", Codec.forName("Lucene40")); in AppendingCodec's constructor, I get a:

        [junit4:junit4]    > Throwable #1: java.util.ServiceConfigurationError: Cannot instantiate SPI class: org.apache.lucene.codecs.appending.AppendingCodec
        [junit4:junit4]    > 	at org.apache.lucene.util.NamedSPILoader.reload(NamedSPILoader.java:72)
        [junit4:junit4]    > 	at org.apache.lucene.util.NamedSPILoader.<init>(NamedSPILoader.java:42)
        [junit4:junit4]    > 	at org.apache.lucene.util.NamedSPILoader.<init>(NamedSPILoader.java:37)
        [junit4:junit4]    > 	at org.apache.lucene.codecs.Codec.<clinit>(Codec.java:39)
        [junit4:junit4]    > 	at org.apache.lucene.util.TestRuleSetupAndRestoreClassEnv.before(TestRuleSetupAndRestoreClassEnv.java:133)
        [junit4:junit4]    > 	at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:44)
        [junit4:junit4]    > 	at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:42)
        [junit4:junit4]    > 	at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55)
        [junit4:junit4]    > 	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
        [junit4:junit4]    > 	at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39)
        [junit4:junit4]    > 	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
        [junit4:junit4]    > 	at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:43)
        [junit4:junit4]    > 	at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48)
        [junit4:junit4]    > 	at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70)
        [junit4:junit4]    > 	at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:55)
        [junit4:junit4]    > 	at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
        [junit4:junit4]    > 	at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358)
        [junit4:junit4]    > 	at java.lang.Thread.run(Thread.java:722)
        [junit4:junit4]    > Caused by: java.lang.NullPointerException
        [junit4:junit4]    > 	at org.apache.lucene.codecs.Codec.forName(Codec.java:89)
        [junit4:junit4]    > 	at org.apache.lucene.codecs.appending.AppendingCodec.<init>(AppendingCodec.java:35)
        [junit4:junit4]    > 	at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        [junit4:junit4]    > 	at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57)
        [junit4:junit4]    > 	at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        [junit4:junit4]    > 	at java.lang.reflect.Constructor.newInstance(Constructor.java:525)
        [junit4:junit4]    > 	at java.lang.Class.newInstance0(Class.java:372)
        [junit4:junit4]    > 	at java.lang.Class.newInstance(Class.java:325)
        [junit4:junit4]    > 	at org.apache.lucene.util.NamedSPILoader.reload(NamedSPILoader.java:62)
        [junit4:junit4]    > 	... 17 more
        

        )

        So I thought it would be less error-prone to let users specify the delegate through an abstract method?

        Show
        Adrien Grand added a comment - The reason I used a delegate() method is that setting the delegate in the constructor seems to generate cyclic dependencies when used in combination with Codec.forName . (For example, if I use super("Appending", Codec.forName("Lucene40")); in AppendingCodec's constructor, I get a: [junit4:junit4] > Throwable #1: java.util.ServiceConfigurationError: Cannot instantiate SPI class: org.apache.lucene.codecs.appending.AppendingCodec [junit4:junit4] > at org.apache.lucene.util.NamedSPILoader.reload(NamedSPILoader.java:72) [junit4:junit4] > at org.apache.lucene.util.NamedSPILoader.<init>(NamedSPILoader.java:42) [junit4:junit4] > at org.apache.lucene.util.NamedSPILoader.<init>(NamedSPILoader.java:37) [junit4:junit4] > at org.apache.lucene.codecs.Codec.<clinit>(Codec.java:39) [junit4:junit4] > at org.apache.lucene.util.TestRuleSetupAndRestoreClassEnv.before(TestRuleSetupAndRestoreClassEnv.java:133) [junit4:junit4] > at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:44) [junit4:junit4] > at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:42) [junit4:junit4] > at com.carrotsearch.randomizedtesting.rules.SystemPropertiesInvariantRule$1.evaluate(SystemPropertiesInvariantRule.java:55) [junit4:junit4] > at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39) [junit4:junit4] > at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:39) [junit4:junit4] > at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) [junit4:junit4] > at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:43) [junit4:junit4] > at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:48) [junit4:junit4] > at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:70) [junit4:junit4] > at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:55) [junit4:junit4] > at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36) [junit4:junit4] > at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:358) [junit4:junit4] > at java.lang.Thread.run(Thread.java:722) [junit4:junit4] > Caused by: java.lang.NullPointerException [junit4:junit4] > at org.apache.lucene.codecs.Codec.forName(Codec.java:89) [junit4:junit4] > at org.apache.lucene.codecs.appending.AppendingCodec.<init>(AppendingCodec.java:35) [junit4:junit4] > at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) [junit4:junit4] > at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57) [junit4:junit4] > at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) [junit4:junit4] > at java.lang.reflect.Constructor.newInstance(Constructor.java:525) [junit4:junit4] > at java.lang.Class.newInstance0(Class.java:372) [junit4:junit4] > at java.lang.Class.newInstance(Class.java:325) [junit4:junit4] > at org.apache.lucene.util.NamedSPILoader.reload(NamedSPILoader.java:62) [junit4:junit4] > ... 17 more ) So I thought it would be less error-prone to let users specify the delegate through an abstract method?
        Hide
        Adrien Grand added a comment -

        Same patch, but with ForwardingCodec renamed as FilterCodec. I however didn't change the way it gets the delegate codec because of the issue mentioned above.

        Show
        Adrien Grand added a comment - Same patch, but with ForwardingCodec renamed as FilterCodec. I however didn't change the way it gets the delegate codec because of the issue mentioned above.
        Hide
        Adrien Grand added a comment -

        Just committed (r1387222 on trunk and r1387229 on branch 4.x).

        Show
        Adrien Grand added a comment - Just committed (r1387222 on trunk and r1387229 on branch 4.x).
        Hide
        Shai Erera added a comment -

        I really don't like the delegate() solution ... every method calls this, which in turn calls Codec.forName(). I have a solution and a simple patch to this. I'll open a separate issue as it's focused just on FilterCodec.

        Show
        Shai Erera added a comment - I really don't like the delegate() solution ... every method calls this, which in turn calls Codec.forName(). I have a solution and a simple patch to this. I'll open a separate issue as it's focused just on FilterCodec.
        Hide
        Commit Tag Bot added a comment -

        [branch_4x commit] Adrien Grand
        http://svn.apache.org/viewvc?view=revision&revision=1387229

        LUCENE-4391: Make Lucene40Codec's methods final (merged from r1387222).

        Show
        Commit Tag Bot added a comment - [branch_4x commit] Adrien Grand http://svn.apache.org/viewvc?view=revision&revision=1387229 LUCENE-4391 : Make Lucene40Codec's methods final (merged from r1387222).
        Hide
        Uwe Schindler added a comment -

        Closed after release.

        Show
        Uwe Schindler added a comment - Closed after release.

          People

          • Assignee:
            Adrien Grand
            Reporter:
            Adrien Grand
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development