Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.0-ALPHA, 5.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      A while ago there was a discussion about making some IW settings "live" and I remember that RAM buffer size was one of them. Judging from IW code, I see that RAM buffer can be changed "live" as IW never caches it.

      However, I don't remember which other settings were decided to be "live" and I don't see any documentation in IW nor IWC for that. IW.getConfig mentions:

      * <b>NOTE:</b> some settings may be changed on the
      * returned {@link IndexWriterConfig}, and will take
      * effect in the current IndexWriter instance.  See the
      * javadocs for the specific setters in {@link
      * IndexWriterConfig} for details.
      

      But there's no text on e.g. IWC.setRAMBuffer mentioning that.

      I think that it'd be good if we make it easier for users to tell which of the settings are "live" ones. There are few possible ways to do it:

      • Introduce a custom @live.setting tag on the relevant IWC.set methods, and add special text for them in build.xml
        • Or, drop the tag and just document it clearly.
      • Separate IWC to two interfaces, LiveConfig and OneTimeConfig (name proposals are welcome !), have IWC impl both, and introduce another IW.getLiveConfig which will return that interface, thereby clearly letting the user know which of the settings are "live".

      It'd be good if IWC itself could only expose setXYZ methods for the "live" settings though. So perhaps, off the top of my head, we can do something like this:

      • Introduce a Config object, which is essentially what IWC is today, and pass it to IW.
      • IW will create a different object, IWC from that Config and IW.getConfig will return IWC.
      • IWC itself will only have setXYZ methods for the "live" settings.

      It adds another object, but user code doesn't change - it still creates a Config object when initializing IW, and need to handle a different type if it ever calls IW.getConfig.

      Maybe that's not such a bad idea?

      1. LUCENE-4132.patch
        58 kB
        Shai Erera
      2. LUCENE-4132.patch
        58 kB
        Shai Erera
      3. LUCENE-4132.patch
        57 kB
        Shai Erera
      4. LUCENE-4132.patch
        55 kB
        Shai Erera
      5. LUCENE-4132.patch
        54 kB
        Shai Erera
      6. LUCENE-4132.patch
        57 kB
        Shai Erera
      7. LUCENE-4132.patch
        59 kB
        Shai Erera

        Activity

        Shai Erera created issue -
        Hide
        Robert Muir added a comment -

        I don't think we should add another Config object, making things complicated for such a very very expert use case.
        Even ordinary users need to use IWC, and 99% of them don't care about changing things live.

        I'm also nervous about documenting which things can/cannot be changed live unless there are unit tests for each one.
        If we want to refactor indexwriter in some way that really cleans it up, but makes something "un-live", then I think
        thats totally fair game and we should be able to do it, but the docs shouldnt be wrong.

        Show
        Robert Muir added a comment - I don't think we should add another Config object, making things complicated for such a very very expert use case. Even ordinary users need to use IWC, and 99% of them don't care about changing things live. I'm also nervous about documenting which things can/cannot be changed live unless there are unit tests for each one. If we want to refactor indexwriter in some way that really cleans it up, but makes something "un-live", then I think thats totally fair game and we should be able to do it, but the docs shouldnt be wrong.
        Hide
        Shai Erera added a comment -

        I don't think we should add another Config object, making things complicated for such a very very expert use case.
        Even ordinary users need to use IWC, and 99% of them don't care about changing things live.

        I'm not proposing to complicate matters for 99.9% of the users. On the contrary – users will still do:

        IndexWriterConfig config = new IndexWriterConfig(...);
        // configure it
        IndexWriter writer = new IndexWriter(dir, config);
        

        Only the expert users who will want to change some settings "live", will do:

        Config conf = writer.getConfig(); // NOTE: it's a different type
        conf.setSomething();
        

        Config can be an IW internal type and most users won't even be aware of it. Today we document that the given IWC to IW ctor is cloned and it will remain as such. Only instead of being cloned to an IWC type, it will be cloned to a Config (or LiveConfig) type.

        IWC documentation isn't changed, IW.getConfig changes by removing that NOTE, and if you care about "lively" configure IW, you can do so through LiveConfig. And we can test that type too !

        Show
        Shai Erera added a comment - I don't think we should add another Config object, making things complicated for such a very very expert use case. Even ordinary users need to use IWC, and 99% of them don't care about changing things live. I'm not proposing to complicate matters for 99.9% of the users. On the contrary – users will still do: IndexWriterConfig config = new IndexWriterConfig(...); // configure it IndexWriter writer = new IndexWriter(dir, config); Only the expert users who will want to change some settings "live", will do: Config conf = writer.getConfig(); // NOTE: it's a different type conf.setSomething(); Config can be an IW internal type and most users won't even be aware of it. Today we document that the given IWC to IW ctor is cloned and it will remain as such. Only instead of being cloned to an IWC type, it will be cloned to a Config (or LiveConfig) type. IWC documentation isn't changed, IW.getConfig changes by removing that NOTE, and if you care about "lively" configure IW, you can do so through LiveConfig. And we can test that type too !
        Hide
        Robert Muir added a comment -

        Right, but i suppose changing live settings isnt necessarily the only use case for writer.getConfig() ?

        Today someone can take the config off of there and set it on another writer (it will be privately cloned).
        so i think if we want to do it this way, we could just keep getConfig as is, and add getLiveConfig which
        actually returns the same object, just cast through that interface.

        Show
        Robert Muir added a comment - Right, but i suppose changing live settings isnt necessarily the only use case for writer.getConfig() ? Today someone can take the config off of there and set it on another writer (it will be privately cloned). so i think if we want to do it this way, we could just keep getConfig as is, and add getLiveConfig which actually returns the same object, just cast through that interface.
        Hide
        Robert Muir added a comment -

        ok actually i was partially wrong, one can no longer actually use the IWC from a writer since its marked as "owned".
        But they can still grab it and look at stuff like getIndexDeletionPolicy, even though thats not live.

        I guess to be less confusing we should add getLiveConfig and just remove getConfig completely?

        Show
        Robert Muir added a comment - ok actually i was partially wrong, one can no longer actually use the IWC from a writer since its marked as "owned". But they can still grab it and look at stuff like getIndexDeletionPolicy, even though thats not live. I guess to be less confusing we should add getLiveConfig and just remove getConfig completely?
        Hide
        Shai Erera added a comment -

        Today someone can take the config off of there and set it on another writer (it will be privately cloned)

        True, but I'm not aware of such use, and still someone can cache the IWC himself and pass it to multiple writers?

        If getConfig() returns an IWC which has setters(), that'll confuse the user for sure, because those settings won't take effect.

        I prefer that getConfig return the new LiveConfig type, with few setters and all getters (i.e. all getXYZ from IWC), and let whoever want to pass the same IWC instance to other writers handle it himself.

        Alternatively, we can add another ctor which takes a LiveConfig object, that is returned from getConfig(), but I prefer to avoid that until someone actually tells us that he shares the same IWC with other writers, and he cannot cache it himself?

        Show
        Shai Erera added a comment - Today someone can take the config off of there and set it on another writer (it will be privately cloned) True, but I'm not aware of such use, and still someone can cache the IWC himself and pass it to multiple writers? If getConfig() returns an IWC which has setters(), that'll confuse the user for sure, because those settings won't take effect. I prefer that getConfig return the new LiveConfig type, with few setters and all getters (i.e. all getXYZ from IWC), and let whoever want to pass the same IWC instance to other writers handle it himself. Alternatively, we can add another ctor which takes a LiveConfig object, that is returned from getConfig(), but I prefer to avoid that until someone actually tells us that he shares the same IWC with other writers, and he cannot cache it himself?
        Hide
        Robert Muir added a comment -

        sorry, instead of nuking getConfig make it pkg-private. Things like RandomIndexWriter want to peek into some
        un-live settings (like codec), I think we should still be able to look at these things for tests

        Show
        Robert Muir added a comment - sorry, instead of nuking getConfig make it pkg-private. Things like RandomIndexWriter want to peek into some un-live settings (like codec), I think we should still be able to look at these things for tests
        Hide
        Shai Erera added a comment -

        I guess to be less confusing we should add getLiveConfig and just remove getConfig completely?

        Yes that's the proposal - either getConfig or getLiveConfig, but return a LiveConfig object with all the getters of IWC, and only the setters that we want to support.

        Show
        Shai Erera added a comment - I guess to be less confusing we should add getLiveConfig and just remove getConfig completely? Yes that's the proposal - either getConfig or getLiveConfig, but return a LiveConfig object with all the getters of IWC, and only the setters that we want to support.
        Hide
        Robert Muir added a comment -

        True, but I'm not aware of such use, and still someone can cache the IWC himself and pass it to multiple writers?

        I'm just talking about the general issue that IW.getConfig is not only used to change settings live.
        Today our tests use this to peek at the settings on the IW (see my RandomIndexWriter example)...

        Show
        Robert Muir added a comment - True, but I'm not aware of such use, and still someone can cache the IWC himself and pass it to multiple writers? I'm just talking about the general issue that IW.getConfig is not only used to change settings live. Today our tests use this to peek at the settings on the IW (see my RandomIndexWriter example)...
        Hide
        Shai Erera added a comment - - edited

        Phew, that was tricky, but here's the end result – refactored IndexWriterConfig into the following class hierarchy:

        • ReadOnlyConfig
          • AbstractLiveConfig
            • LiveConfig
            • IndexWriterConfig
        • IndexWriter now takes ReadOnlyConfig, which is an abstract class with all abstract getters.
        • LiveConfig is returned from IndexWriter.getConfig(), and is initialized with the ReadOnlyConfig given to IW. It overrides all getters to delegate the call to the given (cloned) config. It is public but with a package-private ctor.
        • IndexWriterConfig is still the entry object for users to initialize an IndexWriter, and adds its own setters for the non-live settings.
        • The AbstractLiveConfig in the middle is used for generics and keeping the builder pattern. That way, LiveConfig.set1() and IndexWriterConfig.set1() return the proper type (LiveConfig or IndexWriterConfig respectively).

        I would have liked IW to keep getting IWC in its ctor, but there's one test that prevents it: TestIndexWriterConfig.testIWCInvalidReuse, which initializes an IW, call getConfig and passes it to another IW (which is invalid). I don't know why it's invalid, as IW clones the given IWC, but that is one reason why I had to factor the getters out to a shared ReadOnlyConfig.

        ROC is not that bad though – it kind of protects against IW changing the given config ...

        At least, no user code should change following these changes, except from changing the variable type used to cache IW.getConfig() to LiveConfig, which is what we want.

        Show
        Shai Erera added a comment - - edited Phew, that was tricky, but here's the end result – refactored IndexWriterConfig into the following class hierarchy: ReadOnlyConfig AbstractLiveConfig LiveConfig IndexWriterConfig IndexWriter now takes ReadOnlyConfig, which is an abstract class with all abstract getters. LiveConfig is returned from IndexWriter.getConfig(), and is initialized with the ReadOnlyConfig given to IW. It overrides all getters to delegate the call to the given (cloned) config. It is public but with a package-private ctor. IndexWriterConfig is still the entry object for users to initialize an IndexWriter, and adds its own setters for the non-live settings. The AbstractLiveConfig in the middle is used for generics and keeping the builder pattern. That way, LiveConfig.set1() and IndexWriterConfig.set1() return the proper type (LiveConfig or IndexWriterConfig respectively). I would have liked IW to keep getting IWC in its ctor, but there's one test that prevents it: TestIndexWriterConfig.testIWCInvalidReuse, which initializes an IW, call getConfig and passes it to another IW (which is invalid). I don't know why it's invalid, as IW clones the given IWC, but that is one reason why I had to factor the getters out to a shared ReadOnlyConfig. ROC is not that bad though – it kind of protects against IW changing the given config ... At least, no user code should change following these changes, except from changing the variable type used to cache IW.getConfig() to LiveConfig, which is what we want.
        Shai Erera made changes -
        Field Original Value New Value
        Attachment LUCENE-4132.patch [ 12531843 ]
        Hide
        Shai Erera added a comment -

        I had a brief chat about IWC.usedByIW with Mike (was introduced in LUCENE-4084), and we both agree it's not needed anymore, as now with IW.getConfig() returning LiveConfig and IW taking IWC in its ctor, no one can pass the same instance returned from getConfig to a new IW, and so the relevant test can be nuked, together with that AtomicBoolean.

        I'll nuke them then, and absorb ReadOnlyConfig into AbstractLiveConfig and stick with just two concrete clases: LiveConfig returned from IW.getConfig and IWC given to its ctor.

        I'll post a patch probably tomorrow.

        Show
        Shai Erera added a comment - I had a brief chat about IWC.usedByIW with Mike (was introduced in LUCENE-4084 ), and we both agree it's not needed anymore, as now with IW.getConfig() returning LiveConfig and IW taking IWC in its ctor, no one can pass the same instance returned from getConfig to a new IW, and so the relevant test can be nuked, together with that AtomicBoolean. I'll nuke them then, and absorb ReadOnlyConfig into AbstractLiveConfig and stick with just two concrete clases: LiveConfig returned from IW.getConfig and IWC given to its ctor. I'll post a patch probably tomorrow.
        Hide
        Robert Muir added a comment -

        I think the class hierarchy/generics are too tricky.
        Why do we need generics?

        Show
        Robert Muir added a comment - I think the class hierarchy/generics are too tricky. Why do we need generics?
        Hide
        Uwe Schindler added a comment -

        That's certified and suggested by the generics policeman. The generics are needed to make the builder API work correct (compare Enum<T extends Enum<T>>)

        Show
        Uwe Schindler added a comment - That's certified and suggested by the generics policeman. The generics are needed to make the builder API work correct (compare Enum<T extends Enum<T>>)
        Hide
        Robert Muir added a comment -

        Its not certified by me. Its too confusing for a class everyone must use.

        I dont care about the builder pattern, builder pattern simply isnt worth it for confusing generics on a config class.

        Show
        Robert Muir added a comment - Its not certified by me. Its too confusing for a class everyone must use. I dont care about the builder pattern, builder pattern simply isnt worth it for confusing generics on a config class.
        Hide
        Michael McCandless added a comment -

        If we remove IWC's chained setters (return void), can we simplify this?

        Show
        Michael McCandless added a comment - If we remove IWC's chained setters (return void), can we simplify this?
        Hide
        Uwe Schindler added a comment -

        We could, I am against, I love IndexWriterConfig!

        Show
        Uwe Schindler added a comment - We could, I am against, I love IndexWriterConfig!
        Hide
        Shai Erera added a comment -

        I love it too, and the changes would be too horrible. We use this builder pattern everywhere. Remember, the changes in this issue are to not confuse people, that's it. They don't cause users to change their code almost at all.

        I don't quite understand what's the issue with the generics. If you don't look at IWC / LC code, it's not visible at all. I mean, in your application code, you won't see any generics.

        Show
        Shai Erera added a comment - I love it too, and the changes would be too horrible. We use this builder pattern everywhere. Remember, the changes in this issue are to not confuse people, that's it. They don't cause users to change their code almost at all. I don't quite understand what's the issue with the generics. If you don't look at IWC / LC code, it's not visible at all. I mean, in your application code, you won't see any generics.
        Hide
        Shai Erera added a comment -

        The generics is because I wanted to not duplicate code between LiveConfig and IWC, so that the live settings share the same setXYZ code. First I thought to write a separate LiveConfig class, but then the setter methods need to be duplicated. I'll take another look – perhaps IWC.setRAMBuffer for instance can just delegate to a private LiveConfig instance.setter. That will keep the APIs without generics, with perhaps so jdoc duplication ...

        I can take a stab at something like that, or if you have another proposal. I don't want to let go of the builder pattern though.

        Show
        Shai Erera added a comment - The generics is because I wanted to not duplicate code between LiveConfig and IWC, so that the live settings share the same setXYZ code. First I thought to write a separate LiveConfig class, but then the setter methods need to be duplicated. I'll take another look – perhaps IWC.setRAMBuffer for instance can just delegate to a private LiveConfig instance.setter. That will keep the APIs without generics, with perhaps so jdoc duplication ... I can take a stab at something like that, or if you have another proposal. I don't want to let go of the builder pattern though.
        Hide
        Simon Willnauer added a comment -

        I think the generics here are not very complicated and also not really user facing. its only a tool here to make things nice for the user I think that justifies it. so I think this looks good though.

        Show
        Simon Willnauer added a comment - I think the generics here are not very complicated and also not really user facing. its only a tool here to make things nice for the user I think that justifies it. so I think this looks good though.
        Hide
        Michael McCandless added a comment -

        I dislike chaining ("return this" from setters) since it's so easily and often abused. Here's an example from Lucene's tests:

            IndexWriter w = new MockIndexWriter(dir, newIndexWriterConfig(
                TEST_VERSION_CURRENT, new MockAnalyzer(random())).setOpenMode(OpenMode.CREATE)
                     .setRAMBufferSizeMB(0.1).setMaxBufferedDocs(maxBufferedDocs).setIndexerThreadPool(new ThreadAffinityDocumentsWriterThreadPool(maxThreadStates))
                     .setReaderPooling(doReaderPooling).setMergePolicy(newLogMergePolicy()));
        

        I don't quite understand what's the issue with the generics. If you don't look at IWC / LC code, it's not visible at all. I mean, in your application code, you won't see any generics.

        It's also important to keep our non-user-facing sources simple too, so
        our source code is approachable to new users.

        Having 4 classes, plus generics, for what ought to be a simple
        configuration class, is too much in my opinion. I'd rather stick with
        javadocs explaining what can and cannot be changed live. This (wanting
        to change live seettings) is expert usage...

        Hopefully, we can simplify the approach here.

        Show
        Michael McCandless added a comment - I dislike chaining ("return this" from setters) since it's so easily and often abused. Here's an example from Lucene's tests: IndexWriter w = new MockIndexWriter(dir, newIndexWriterConfig( TEST_VERSION_CURRENT, new MockAnalyzer(random())).setOpenMode(OpenMode.CREATE) .setRAMBufferSizeMB(0.1).setMaxBufferedDocs(maxBufferedDocs).setIndexerThreadPool(new ThreadAffinityDocumentsWriterThreadPool(maxThreadStates)) .setReaderPooling(doReaderPooling).setMergePolicy(newLogMergePolicy())); I don't quite understand what's the issue with the generics. If you don't look at IWC / LC code, it's not visible at all. I mean, in your application code, you won't see any generics. It's also important to keep our non-user-facing sources simple too, so our source code is approachable to new users. Having 4 classes, plus generics, for what ought to be a simple configuration class, is too much in my opinion. I'd rather stick with javadocs explaining what can and cannot be changed live. This (wanting to change live seettings) is expert usage... Hopefully, we can simplify the approach here.
        Hide
        Shai Erera added a comment -

        Patch removes ReadOnlyConfig and the tests from TestIWC that ensure that the same IWC instance isn't shared between IWs. This is no longer needed, since now IW takes IWC and returns LiveConfig, so it cannot be passed to another IW, simply because the compiler won't allow it.

        This is a better solution IMO than maintaining an AtomicBoolean + tests that enforce that + RuntimeException that is known only during testing, or worse.

        I don't think we should disable the Builder pattern - our tests use it, and I bet users use it too (my code does). If it bothers anyone, he can separately change all our tests to call the setters one per line.

        The generics, as Simon said, are just a tool to save code duplication and make sure that IWC and LC have the same getter signatures, and share the live setters.

        The fact is, no user code will change by that change, and really, no Lucene developer should be affected by it either – this is just a configuration class, adding set/get methods to it will be as easy as they were before. But now compile-wise, we don't let even expert users change non-live settings.

        Show
        Shai Erera added a comment - Patch removes ReadOnlyConfig and the tests from TestIWC that ensure that the same IWC instance isn't shared between IWs. This is no longer needed, since now IW takes IWC and returns LiveConfig, so it cannot be passed to another IW, simply because the compiler won't allow it. This is a better solution IMO than maintaining an AtomicBoolean + tests that enforce that + RuntimeException that is known only during testing, or worse. I don't think we should disable the Builder pattern - our tests use it, and I bet users use it too (my code does). If it bothers anyone, he can separately change all our tests to call the setters one per line. The generics, as Simon said, are just a tool to save code duplication and make sure that IWC and LC have the same getter signatures, and share the live setters. The fact is, no user code will change by that change, and really, no Lucene developer should be affected by it either – this is just a configuration class, adding set/get methods to it will be as easy as they were before. But now compile-wise, we don't let even expert users change non-live settings.
        Shai Erera made changes -
        Attachment LUCENE-4132.patch [ 12531969 ]
        Hide
        Shai Erera added a comment -

        If there are no objections, I'd like to commit this by tomorrow.

        Show
        Shai Erera added a comment - If there are no objections, I'd like to commit this by tomorrow.
        Hide
        Robert Muir added a comment -

        I still feel the same way as before. I'm sorry you don't agree with me, but please don't shove the change in.

        Show
        Robert Muir added a comment - I still feel the same way as before. I'm sorry you don't agree with me, but please don't shove the change in.
        Hide
        Michael McCandless added a comment -

        Please don't rush this Shai.

        I still don't like the added complexity of the current patch. I do
        think compile-time checking of live configuration would be neat/nice to
        have (for expert users) but not at the cost of the added complexity
        (abstract classes, generics) of the current patch.

        This is too much for what should be (is, today) a simple configuration
        class. I'd rather keep what we have today.

        Maybe we can somehow simplify the patch while enabling strong type
        checking of what's live and what isn't? Or, we can enable the type
        checking, but dynamically at runtime; this way at least you'd get an
        exception if you tried to change something. Or, stop chainging (return
        void from all setters)... then the subclassing is straightforward. Or,
        we simply improve javadocs. All of these options would be an
        improvement.

        Or, we just keep what we have today... changing live settings is an
        expert use case. We shouldn't make our code more complex for it.

        You've already done the hardest part here (figuring out what is live and
        what isn't)!

        Show
        Michael McCandless added a comment - Please don't rush this Shai. I still don't like the added complexity of the current patch. I do think compile-time checking of live configuration would be neat/nice to have (for expert users) but not at the cost of the added complexity (abstract classes, generics) of the current patch. This is too much for what should be (is, today) a simple configuration class. I'd rather keep what we have today. Maybe we can somehow simplify the patch while enabling strong type checking of what's live and what isn't? Or, we can enable the type checking, but dynamically at runtime; this way at least you'd get an exception if you tried to change something. Or, stop chainging (return void from all setters)... then the subclassing is straightforward. Or, we simply improve javadocs. All of these options would be an improvement. Or, we just keep what we have today... changing live settings is an expert use case. We shouldn't make our code more complex for it. You've already done the hardest part here (figuring out what is live and what isn't)!
        Hide
        Shai Erera added a comment -

        Sorry if it came across like that, but I don't mean to rush or shove this issue in. I'm usually after consensus and I appreciate your feedback.

        I took another look at this, and found a solution without generics. Funny thing is, that's the first solution that came to my mind, but I guess at the time it didn't picture well enough, so I discarded it .

        Now we have only LiveConfig and IndexWriterConfig, where IWC extends LC and overrides all setter methods. The "live" setters are overridden just to return IWC type, and call super.setXYZ(). So we don't have code dup, and whoever has IWC type at hand, will receive IWC back from all set() methods.

        LC is public class but with package-private ctors, one that takes IWC (used by IndexWriter) and one that takes Analyzer+Version, to match IWC's. It contains all "live" members as private, and the others as protected, so that IWC can set them. Since it cannot be sub-classed outside the package, this is 'safe'.

        The only thing that bothers me, and I'm not sure if it can be fixed, but this is not critical either, is TestIWC.testSettersChaining(). For some reason, even though I override the setters from LC in IWC, and set their return type to IWC, reflection still returns their return type as LiveConfig. This only affects the test, since if I do:

        IndexWriterConfig conf;
        conf.setMaxBufferedDocs(); // or any other set from LC
        

        the return type is IWC.

        If anyone knows how to solve it, please let me know, otherwise we'll just have to live with the modification to the test, and the chance that future "live" setters may be incorrectly overridden by IWC to not return IWC type That is not an error, just a convenience.

        Besides that, and if I follow your comments and concerns properly, I think this is now ready to commit – there's no extra complexity (generics, 3 classes etc.), and with better compile time protection against misuse.

        Show
        Shai Erera added a comment - Sorry if it came across like that, but I don't mean to rush or shove this issue in. I'm usually after consensus and I appreciate your feedback. I took another look at this, and found a solution without generics. Funny thing is, that's the first solution that came to my mind, but I guess at the time it didn't picture well enough, so I discarded it . Now we have only LiveConfig and IndexWriterConfig, where IWC extends LC and overrides all setter methods. The "live" setters are overridden just to return IWC type, and call super.setXYZ(). So we don't have code dup, and whoever has IWC type at hand, will receive IWC back from all set() methods. LC is public class but with package-private ctors, one that takes IWC (used by IndexWriter) and one that takes Analyzer+Version, to match IWC's. It contains all "live" members as private, and the others as protected, so that IWC can set them. Since it cannot be sub-classed outside the package, this is 'safe'. The only thing that bothers me, and I'm not sure if it can be fixed, but this is not critical either, is TestIWC.testSettersChaining(). For some reason, even though I override the setters from LC in IWC, and set their return type to IWC, reflection still returns their return type as LiveConfig. This only affects the test, since if I do: IndexWriterConfig conf; conf.setMaxBufferedDocs(); // or any other set from LC the return type is IWC. If anyone knows how to solve it, please let me know, otherwise we'll just have to live with the modification to the test, and the chance that future "live" setters may be incorrectly overridden by IWC to not return IWC type That is not an error, just a convenience. Besides that, and if I follow your comments and concerns properly, I think this is now ready to commit – there's no extra complexity (generics, 3 classes etc.), and with better compile time protection against misuse.
        Shai Erera made changes -
        Attachment LUCENE-4132.patch [ 12532047 ]
        Hide
        Uwe Schindler added a comment -

        Hi Shai,

        ignore all methods with isSynthetic() set (that are covariant overrides compatibility methods, access$xx() methods for access to private fields/ctors/...).

        Show
        Uwe Schindler added a comment - Hi Shai, ignore all methods with isSynthetic() set (that are covariant overrides compatibility methods, access$xx() methods for access to private fields/ctors/...).
        Hide
        Shai Erera added a comment -

        Thanks Uwe. The test is now fixed by saving all 'synthetic' methods and all 'setter' methods and verifying in the end that all of them were received from IWC too.

        Show
        Shai Erera added a comment - Thanks Uwe. The test is now fixed by saving all 'synthetic' methods and all 'setter' methods and verifying in the end that all of them were received from IWC too.
        Shai Erera made changes -
        Attachment LUCENE-4132.patch [ 12532049 ]
        Hide
        Robert Muir added a comment -

        Can we override all methods so the javadocs aren't confusing.

        I don't want the methods split in the javadocs between IWC and LiveConfig: LiveConfig is expert and should be a subset, not a portion.

        Show
        Robert Muir added a comment - Can we override all methods so the javadocs aren't confusing. I don't want the methods split in the javadocs between IWC and LiveConfig: LiveConfig is expert and should be a subset, not a portion.
        Hide
        Michael McCandless added a comment -

        Also can we rename it to LiveIndexWriterConfig? LiveConfig is too generic I think...

        Show
        Michael McCandless added a comment - Also can we rename it to LiveIndexWriterConfig? LiveConfig is too generic I think...
        Hide
        Shai Erera added a comment -

        Can we override all methods so the javadocs aren't confusing.

        Good idea! Done

        Also can we rename it to LiveIndexWriterConfig?

        Done

        Show
        Shai Erera added a comment - Can we override all methods so the javadocs aren't confusing. Good idea! Done Also can we rename it to LiveIndexWriterConfig? Done
        Shai Erera made changes -
        Attachment LUCENE-4132.patch [ 12532076 ]
        Hide
        Robert Muir added a comment -

        thanks, +1

        Show
        Robert Muir added a comment - thanks, +1
        Hide
        Shai Erera added a comment -

        Thanks Robert. I'll wait until Sunday and commit it.

        Show
        Shai Erera added a comment - Thanks Robert. I'll wait until Sunday and commit it.
        Shai Erera made changes -
        Assignee Shai Erera [ shaie ]
        Hide
        Michael McCandless added a comment -

        Hmm we are no longer cloning the IWC passed into IW? Maybe we shouldn't remove testReuse?

        Show
        Michael McCandless added a comment - Hmm we are no longer cloning the IWC passed into IW? Maybe we shouldn't remove testReuse?
        Hide
        Shai Erera added a comment -

        Good catch Mike ! It went away in the last changes. I re-added testReuse, with asserting that e.g. the MP instances returned from LiveIWC are not the same.

        Show
        Shai Erera added a comment - Good catch Mike ! It went away in the last changes. I re-added testReuse, with asserting that e.g. the MP instances returned from LiveIWC are not the same.
        Shai Erera made changes -
        Attachment LUCENE-4132.patch [ 12532161 ]
        Hide
        Michael McCandless added a comment -

        I re-added testReuse, with asserting that e.g. the MP instances returned from LiveIWC are not the same.

        Thanks!

        Shouldn't clone() be protected in LiveIndexWriterConfig and public only in IndexWriterConfig?

        Ie, users should never have any reason to clone the live config?

        Show
        Michael McCandless added a comment - I re-added testReuse, with asserting that e.g. the MP instances returned from LiveIWC are not the same. Thanks! Shouldn't clone() be protected in LiveIndexWriterConfig and public only in IndexWriterConfig? Ie, users should never have any reason to clone the live config?
        Hide
        Shai Erera added a comment - - edited

        Shouldn't clone() be protected in LiveIndexWriterConfig and public only in IndexWriterConfig?

        I guess you're right. In fact, I think that LiveIWC should not even be Cloneable, only IWC should? I'll take a look later when I'm near the code.

        Show
        Shai Erera added a comment - - edited Shouldn't clone() be protected in LiveIndexWriterConfig and public only in IndexWriterConfig? I guess you're right. In fact, I think that LiveIWC should not even be Cloneable, only IWC should? I'll take a look later when I'm near the code.
        Hide
        Shai Erera added a comment -

        Patch removes Cloenable from LiveIWC. Now only IWC is Cloenable.

        Show
        Shai Erera added a comment - Patch removes Cloenable from LiveIWC. Now only IWC is Cloenable.
        Shai Erera made changes -
        Attachment LUCENE-4132.patch [ 12532331 ]
        Hide
        Shai Erera added a comment -

        Committed revision 1351225 (trunk) and revision 1351229 (4x).

        Thanks all for your comments !

        Show
        Shai Erera added a comment - Committed revision 1351225 (trunk) and revision 1351229 (4x). Thanks all for your comments !
        Shai Erera made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Lucene Fields New [ 10121 ] New,Patch Available [ 10121, 10120 ]
        Resolution Fixed [ 1 ]
        Hide
        Robert Muir added a comment -

        CHANGES.txt is mangled

        Show
        Robert Muir added a comment - CHANGES.txt is mangled
        Robert Muir made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Hide
        Uwe Schindler added a comment -

        The 4.x commit also breaks the builds, looks like duplicate merges to same file.

        Show
        Uwe Schindler added a comment - The 4.x commit also breaks the builds, looks like duplicate merges to same file.
        Hide
        Uwe Schindler added a comment -

        The 4.x commit also breaks the builds, looks like duplicate merges to same file.

        This was solved by cleaning workspace on Apache's Jenkins. Must have been a SVN problem.

        Show
        Uwe Schindler added a comment - The 4.x commit also breaks the builds, looks like duplicate merges to same file. This was solved by cleaning workspace on Apache's Jenkins. Must have been a SVN problem.
        Hide
        Hoss Man added a comment -

        bulk cleanup of 4.0-ALPHA / 4.0 Jira versioning. all bulk edited issues have hoss20120711-bulk-40-change in a comment

        Show
        Hoss Man added a comment - bulk cleanup of 4.0-ALPHA / 4.0 Jira versioning. all bulk edited issues have hoss20120711-bulk-40-change in a comment
        Hoss Man made changes -
        Fix Version/s 4.0 [ 12322456 ]
        Fix Version/s 4.0-ALPHA [ 12314025 ]
        Fix Version/s 5.0 [ 12321663 ]
        Hide
        Shai Erera added a comment -

        This should have been changed back to 'resolved' a long time ago. I guess I missed it.

        Show
        Shai Erera added a comment - This should have been changed back to 'resolved' a long time ago. I guess I missed it.
        Shai Erera made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Fix Version/s 5.0 [ 12321663 ]
        Fix Version/s 4.0-ALPHA [ 12314025 ]
        Fix Version/s 4.0 [ 12322456 ]
        Resolution Fixed [ 1 ]
        Uwe Schindler made changes -
        Status Resolved [ 5 ] Closed [ 6 ]

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development