Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      See LUCENE-5527 for more context: several of us seem to prefer Leaf to Atomic.

      Talking from my experience, I was a bit confused in the beginning that this thing is named AtomicReader, since Atomic is otherwise used in Java in the context of concurrency. So maybe renaming it to Leaf would help remove this confusion and also carry the information that these readers are used as leaves of top-level readers?

      1. LUCENE-5569.patch
        1.01 MB
        Ryan Ernst
      2. LUCENE-5569.patch
        793 kB
        Ryan Ernst

        Activity

        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        Tim Smith added a comment -

        -1

        please don't do this

        renaming things for the sake of renaming them is a horrible burden on people using these apis

        for instance, every single minor version of lucene 4.x has broken api signatures, resulting in hours, or days worth of time to reconcile the changes

        add in a major name change like this and it adds in significant noise to fixing any real compile errors and significantly complicates the porting process (it took me weeks to upgrade from lucene 3.x to 4.x, i don't want to do that again)

        AtomicReader is a public api in lucene and should not be renamed just because a new name seems better

        Show
        Tim Smith added a comment - -1 please don't do this renaming things for the sake of renaming them is a horrible burden on people using these apis for instance, every single minor version of lucene 4.x has broken api signatures, resulting in hours, or days worth of time to reconcile the changes add in a major name change like this and it adds in significant noise to fixing any real compile errors and significantly complicates the porting process (it took me weeks to upgrade from lucene 3.x to 4.x, i don't want to do that again) AtomicReader is a public api in lucene and should not be renamed just because a new name seems better
        Hide
        Ryan Ernst added a comment -

        +1

        I was also confused like Adrien, thinking atomic had something to do with concurrency. I've attached a patch, just to see what scope this has. It is pretty massive, but it also only took about 2 minutes using IntelliJ.

        I think the major bump to 5 is the perfect time to do this type of renaming. Yes renaming presents a burden on consumers, but refactoring for clarity and ease of entry for new developers outweighs that burden, IMO.

        Show
        Ryan Ernst added a comment - +1 I was also confused like Adrien, thinking atomic had something to do with concurrency. I've attached a patch, just to see what scope this has. It is pretty massive, but it also only took about 2 minutes using IntelliJ. I think the major bump to 5 is the perfect time to do this type of renaming. Yes renaming presents a burden on consumers, but refactoring for clarity and ease of entry for new developers outweighs that burden, IMO.
        Hide
        Robert Muir added a comment -

        Yes 5.0 is the time to fix this stuff. Tim is wrong, its not a horrible burden to users. Users don't have to upgrade. And fixing the api makes it easier on newer users.

        Show
        Robert Muir added a comment - Yes 5.0 is the time to fix this stuff. Tim is wrong, its not a horrible burden to users. Users don't have to upgrade. And fixing the api makes it easier on newer users.
        Hide
        Adrien Grand added a comment -

        +1 to the patch. Maybe we should leave a note about this renaming in the lucene/MIGRATE.txt?

        Show
        Adrien Grand added a comment - +1 to the patch. Maybe we should leave a note about this renaming in the lucene/MIGRATE.txt?
        Hide
        Robert Muir added a comment -

        We should also follow up by completely nuking readercontext, atomicreadercontext. This indirection hurts and complicates all core lucene apis, for all use cases, just to support bad practices and esoteric shit, like climbing up reader tree and using slow wrappers.

        Its ok if we are a little less flexible and simplify the API. For example we could declare readers are instances and have a docbase and parent. Multireaders and other weird shit could wrap the readers to fix this up.

        Show
        Robert Muir added a comment - We should also follow up by completely nuking readercontext, atomicreadercontext. This indirection hurts and complicates all core lucene apis, for all use cases, just to support bad practices and esoteric shit, like climbing up reader tree and using slow wrappers. Its ok if we are a little less flexible and simplify the API. For example we could declare readers are instances and have a docbase and parent. Multireaders and other weird shit could wrap the readers to fix this up.
        Hide
        Adrien Grand added a comment -

        +1 to do that in a follow-up issue

        Show
        Adrien Grand added a comment - +1 to do that in a follow-up issue
        Hide
        Yonik Seeley added a comment -

        FWIW, I agree with Tim. There's not a right or wrong to it... it's a judgement call. It's clear to me that the bar these days to renaming public APIs is far too low... but I appear to be outnumbered.

        Show
        Yonik Seeley added a comment - FWIW, I agree with Tim. There's not a right or wrong to it... it's a judgement call. It's clear to me that the bar these days to renaming public APIs is far too low... but I appear to be outnumbered.
        Hide
        Uwe Schindler added a comment - - edited

        Hi,
        I also tend to aggree with Yonik and Tim. In general I would also prefer to rename it, but the work needed for users is big with having no new features. A pure rename is useless. Unfortunately i was the one who added that name in the big IndexReader refactoring. But at that time we all agreed on that name.
        But we should have thought better about it. LeafReader is fine, especially becaus ethe other APIs already use leave: Like ReaderContext#leaves().
        If we really want to rename that, we should do this shortly before the release of 5.0. Otherwise merging might be much harder from trunk to 4.x. As this is just an eclipse rename of 2 classes (AtomicReader, AtomicReaderContext), this is not much work, just an Eclipse-automatism. The remaining classes is the same: FilterAtomicReader, and various test-only readers.

        Show
        Uwe Schindler added a comment - - edited Hi, I also tend to aggree with Yonik and Tim. In general I would also prefer to rename it, but the work needed for users is big with having no new features. A pure rename is useless. Unfortunately i was the one who added that name in the big IndexReader refactoring. But at that time we all agreed on that name. But we should have thought better about it. LeafReader is fine, especially becaus ethe other APIs already use leave: Like ReaderContext#leaves() . If we really want to rename that, we should do this shortly before the release of 5.0. Otherwise merging might be much harder from trunk to 4.x. As this is just an eclipse rename of 2 classes (AtomicReader, AtomicReaderContext), this is not much work, just an Eclipse-automatism. The remaining classes is the same: FilterAtomicReader, and various test-only readers.
        Hide
        Ryan Ernst added a comment -

        Here is a fresh patch against current trunk. I think we should get this in for 5.0 since branch_5x has been created now.

        Show
        Ryan Ernst added a comment - Here is a fresh patch against current trunk. I think we should get this in for 5.0 since branch_5x has been created now.
        Hide
        Michael McCandless added a comment -

        +1

        Show
        Michael McCandless added a comment - +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1627178 from Ryan Ernst in branch 'dev/trunk'
        [ https://svn.apache.org/r1627178 ]

        LUCENE-5569: *AtomicReader/AtomicReaderContext have been renamed to *LeafReader/LeafReaderContext

        Show
        ASF subversion and git services added a comment - Commit 1627178 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1627178 ] LUCENE-5569 : *AtomicReader/AtomicReaderContext have been renamed to *LeafReader/LeafReaderContext
        Hide
        ASF subversion and git services added a comment -

        Commit 1627188 from Ryan Ernst in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1627188 ]

        LUCENE-5569: *AtomicReader/AtomicReaderContext have been renamed to *LeafReader/LeafReaderContext (merged 1627178)

        Show
        ASF subversion and git services added a comment - Commit 1627188 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1627188 ] LUCENE-5569 : *AtomicReader/AtomicReaderContext have been renamed to *LeafReader/LeafReaderContext (merged 1627178)
        Hide
        ASF subversion and git services added a comment -

        Commit 1627258 from Ryan Ernst in branch 'dev/trunk'
        [ https://svn.apache.org/r1627258 ]

        LUCENE-5569: Rename more locations in test classes and comments

        Show
        ASF subversion and git services added a comment - Commit 1627258 from Ryan Ernst in branch 'dev/trunk' [ https://svn.apache.org/r1627258 ] LUCENE-5569 : Rename more locations in test classes and comments
        Hide
        ASF subversion and git services added a comment -

        Commit 1627266 from Ryan Ernst in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1627266 ]

        LUCENE-5569: Rename more locations in test classes and comments (merged 1627258)

        Show
        ASF subversion and git services added a comment - Commit 1627266 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1627266 ] LUCENE-5569 : Rename more locations in test classes and comments (merged 1627258)
        Hide
        Varun Thacker added a comment -

        Hi Ryan Ernst ,

        I don't see an entry for this in the CHANGES.txt. Should there be one or is it documented somewhere else?

        Show
        Varun Thacker added a comment - Hi Ryan Ernst , I don't see an entry for this in the CHANGES.txt. Should there be one or is it documented somewhere else?
        Hide
        ASF subversion and git services added a comment -

        Commit 1652294 from Ryan Ernst in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1652294 ]

        LUCENE-5569: Backport changes entry

        Show
        ASF subversion and git services added a comment - Commit 1652294 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1652294 ] LUCENE-5569 : Backport changes entry
        Hide
        ASF subversion and git services added a comment -

        Commit 1652295 from Ryan Ernst in branch 'dev/branches/lucene_solr_5_0'
        [ https://svn.apache.org/r1652295 ]

        LUCENE-5569: Backport changes entry (merged 1652294)

        Show
        ASF subversion and git services added a comment - Commit 1652295 from Ryan Ernst in branch 'dev/branches/lucene_solr_5_0' [ https://svn.apache.org/r1652295 ] LUCENE-5569 : Backport changes entry (merged 1652294)
        Hide
        Ryan Ernst added a comment -

        Varun Thacker This was an oversight when doing the branch_5x backport, because it was done independently (without merge). I've copied the changes entry back to branch_5x and lucene_solr_5_0.

        Show
        Ryan Ernst added a comment - Varun Thacker This was an oversight when doing the branch_5x backport, because it was done independently (without merge). I've copied the changes entry back to branch_5x and lucene_solr_5_0 .
        Hide
        Uwe Schindler added a comment -

        Do we have it in 5.0's MIGRATE.txt? Maybe we should place it there, because this may be a major rename for people with lots of custom code.

        Show
        Uwe Schindler added a comment - Do we have it in 5.0's MIGRATE.txt? Maybe we should place it there, because this may be a major rename for people with lots of custom code.
        Hide
        ASF subversion and git services added a comment -

        Commit 1652310 from Ryan Ernst in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1652310 ]

        LUCENE-5569: Add MIGRATE entry for 5.0

        Show
        ASF subversion and git services added a comment - Commit 1652310 from Ryan Ernst in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1652310 ] LUCENE-5569 : Add MIGRATE entry for 5.0
        Hide
        ASF subversion and git services added a comment -

        Commit 1652311 from Ryan Ernst in branch 'dev/branches/lucene_solr_5_0'
        [ https://svn.apache.org/r1652311 ]

        LUCENE-5569: Add MIGRATE entry for 5.0 (merged 1652310)

        Show
        ASF subversion and git services added a comment - Commit 1652311 from Ryan Ernst in branch 'dev/branches/lucene_solr_5_0' [ https://svn.apache.org/r1652311 ] LUCENE-5569 : Add MIGRATE entry for 5.0 (merged 1652310)
        Hide
        Ryan Ernst added a comment -

        Good idea Uwe, I've added a migrate entry.

        Show
        Ryan Ernst added a comment - Good idea Uwe, I've added a migrate entry.
        Hide
        Sanne Grinovero added a comment -

        As a heavy Lucene consumer I probably have no right at all to complain

        But now that the time has come to test the candidate release for 5.0, let me share some facts:

        • This change caused some ~600 compile errors in our codebase
        • My personal opinion being that AtomicReader was a very good name, please take it as a statement that such names are quite a personal choice and someone just needs to make a call (And stick to it!).

        Indeed it's not a major blocker, but as Yonik Seeley wisely puts it, I'd wish the bar against API changes was higher, especially when there isn't a really good reason.

        Show
        Sanne Grinovero added a comment - As a heavy Lucene consumer I probably have no right at all to complain But now that the time has come to test the candidate release for 5.0, let me share some facts: This change caused some ~600 compile errors in our codebase My personal opinion being that AtomicReader was a very good name, please take it as a statement that such names are quite a personal choice and someone just needs to make a call (And stick to it!). Indeed it's not a major blocker, but as Yonik Seeley wisely puts it, I'd wish the bar against API changes was higher, especially when there isn't a really good reason.
        Hide
        Adrien Grand added a comment -

        Actually I think this 5.0 break is one of the easiest ones to fix when upgrading. Although it might generate lots of compilation errors, especially if you use experimental APIs like oal.search.Collector, it's very easy to fix using eg. sed to replace all occurrences of AtomicReader with LeafReader (which will also rename AtomicReaderContext to LeafReaderContext).

        Show
        Adrien Grand added a comment - Actually I think this 5.0 break is one of the easiest ones to fix when upgrading. Although it might generate lots of compilation errors, especially if you use experimental APIs like oal.search.Collector, it's very easy to fix using eg. sed to replace all occurrences of AtomicReader with LeafReader (which will also rename AtomicReaderContext to LeafReaderContext).
        Hide
        Anshum Gupta added a comment -

        Bulk close after 5.0 release.

        Show
        Anshum Gupta added a comment - Bulk close after 5.0 release.

          People

          • Assignee:
            Ryan Ernst
            Reporter:
            Adrien Grand
          • Votes:
            2 Vote for this issue
            Watchers:
            10 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development