Lucene - Core
  1. Lucene - Core
  2. LUCENE-5114

remove boolean useCache param from TermsEnum.seekCeil/Exact

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.5, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      Long ago terms dict had a cache, but it was problematic and we removed it, but the API still has a relic boolean useCache ... I think we should drop it from the API as well.

      1. LUCENE-5114.patch
        83 kB
        Michael McCandless

        Activity

        Hide
        Michael McCandless added a comment -

        Initial patch; tests pass but I haven't run ant precommit yet ...

        Show
        Michael McCandless added a comment - Initial patch; tests pass but I haven't run ant precommit yet ...
        Hide
        Robert Muir added a comment -

        +1 to nuking this parameter!

        Show
        Robert Muir added a comment - +1 to nuking this parameter!
        Hide
        ASF subversion and git services added a comment -

        Commit 1503805 from Michael McCandless in branch 'dev/trunk'
        [ https://svn.apache.org/r1503805 ]

        LUCENE-5114: remove unused useCache param

        Show
        ASF subversion and git services added a comment - Commit 1503805 from Michael McCandless in branch 'dev/trunk' [ https://svn.apache.org/r1503805 ] LUCENE-5114 : remove unused useCache param
        Hide
        ASF subversion and git services added a comment -

        Commit 1503834 from Michael McCandless in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1503834 ]

        LUCENE-5114: remove unused useCache param

        Show
        ASF subversion and git services added a comment - Commit 1503834 from Michael McCandless in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1503834 ] LUCENE-5114 : remove unused useCache param
        Hide
        David Smiley added a comment -

        I'm very supportive of this change. However, isn't this a breaking change you committed to 4x? On 4x, might it make sense to leave these as overloaded deprecated methods? I think so.

        Show
        David Smiley added a comment - I'm very supportive of this change. However, isn't this a breaking change you committed to 4x? On 4x, might it make sense to leave these as overloaded deprecated methods? I think so.
        Hide
        Robert Muir added a comment -

        If you want to "leave" the old method, please please make it final... (otherwise, don't do it)

        In all cases the API must break, or will only invite bugs.

        Show
        Robert Muir added a comment - If you want to "leave" the old method, please please make it final... (otherwise, don't do it) In all cases the API must break, or will only invite bugs.
        Hide
        Michael McCandless added a comment -

        However, isn't this a breaking change you committed to 4x?

        It is a break (I "advertised" this in CHANGES.txt), but these APIs are marked @experimental. Also, I'd rather on upgrade that expert users using these methods realize (because they see the compilation errors) that useCache is/was not doing anything, so they can then think about whether they want to change their code (e.g. add their own cache higher up or something).

        Show
        Michael McCandless added a comment - However, isn't this a breaking change you committed to 4x? It is a break (I "advertised" this in CHANGES.txt), but these APIs are marked @experimental. Also, I'd rather on upgrade that expert users using these methods realize (because they see the compilation errors) that useCache is/was not doing anything, so they can then think about whether they want to change their code (e.g. add their own cache higher up or something).
        Hide
        David Smiley added a comment -

        If you want to "leave" the old method, please please make it final... (otherwise, don't do it)

        +1

        In all cases the API must break, or will only invite bugs.

        What bugs? There wasn't any caching in the first place; it was effectively a no-op.

        Also, I'd rather on upgrade that expert users using these methods realize (because they see the compilation errors) that useCache is/was not doing anything ...

        I think deprecation warnings are fine. Using these methods is not so serious to warrant compiled extensions being incompatible.

        Show
        David Smiley added a comment - If you want to "leave" the old method, please please make it final... (otherwise, don't do it) +1 In all cases the API must break, or will only invite bugs. What bugs? There wasn't any caching in the first place; it was effectively a no-op. Also, I'd rather on upgrade that expert users using these methods realize (because they see the compilation errors) that useCache is/was not doing anything ... I think deprecation warnings are fine. Using these methods is not so serious to warrant compiled extensions being incompatible.
        Hide
        Robert Muir added a comment -

        In all cases the API must break, or will only invite bugs.

        What bugs? There wasn't any caching in the first place; it was effectively a no-op.

        The bugs are when you make a method non-final, then its easy for the "wrong method"
        to be called.

        Sophisticated compatibility can be added (using o.a.l.util.VirtualMethod), but this
        should be reserved for core apis where the change is necessary and we really need
        back compat.

        For experimental apis, don't take the risk! just backwards break.

        Show
        Robert Muir added a comment - In all cases the API must break, or will only invite bugs. What bugs? There wasn't any caching in the first place; it was effectively a no-op. The bugs are when you make a method non-final, then its easy for the "wrong method" to be called. Sophisticated compatibility can be added (using o.a.l.util.VirtualMethod), but this should be reserved for core apis where the change is necessary and we really need back compat. For experimental apis, don't take the risk! just backwards break.
        Hide
        Adrien Grand added a comment -

        4.5 release -> bulk close

        Show
        Adrien Grand added a comment - 4.5 release -> bulk close

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Michael McCandless
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development