Lucene - Core
  1. Lucene - Core
  2. LUCENE-1979

Remove remaining deprecations from indexer package

    Details

    • Type: Task Task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New
    1. lucene-1979.patch
      32 kB
      Michael Busch
    2. lucene-1979.patch
      32 kB
      Michael Busch
    3. LUCENE-1979-2.patch
      47 kB
      Michael Busch
    4. LUCENE-1979-2.patch
      48 kB
      Uwe Schindler
    5. LUCENE-1979-2-bw.patch
      24 kB
      Michael Busch
    6. LUCENE-1979-2-bw.patch
      24 kB
      Uwe Schindler
    7. lucene-1979-bw.patch
      14 kB
      Michael Busch

      Activity

      Hide
      Michael Busch added a comment -

      Removes almost all deprecations from the indexer package. The only things left are:

      • IndexReader#getFieldCacheKey() - what do we do with that one?
      • calls to IndexInput#readChars() and #skipChars() - I think we have to keep those until 4.0?

      All core & contrib tests pass. It'd be good if someone could review this patch though.

      Show
      Michael Busch added a comment - Removes almost all deprecations from the indexer package. The only things left are: IndexReader#getFieldCacheKey() - what do we do with that one? calls to IndexInput#readChars() and #skipChars() - I think we have to keep those until 4.0? All core & contrib tests pass. It'd be good if someone could review this patch though.
      Hide
      Michael McCandless added a comment -

      IndexReader#getFieldCacheKey() - what do we do with that one?

      I think we should undeprecate it. I had deprecated it thinking LUCENE-831 would land.

      Show
      Michael McCandless added a comment - IndexReader#getFieldCacheKey() - what do we do with that one? I think we should undeprecate it. I had deprecated it thinking LUCENE-831 would land.
      Hide
      Michael Busch added a comment -

      OK, will do!

      Show
      Michael Busch added a comment - OK, will do!
      Hide
      Michael Busch added a comment -

      Patch for the back-compat trunk.

      Hmm, everything passes, except this one:

      [junit] java.lang.NoSuchMethodError: org.apache.lucene.index.SnapshotDeletionPolicy.snapshot()Lorg/apache/lucene/index/IndexCommitPoint;
          [junit] 	at org.apache.lucene.TestSnapshotDeletionPolicy.testReuseAcrossWriters(TestSnapshotDeletionPolicy.java:82)
          [junit] 	at org.apache.lucene.util.LuceneTestCase.runBare(LuceneTestCase.java:206)
          [junit] Test org.apache.lucene.TestSnapshotDeletionPolicy FAILED
      

      Here drop-in replacement doesn't seem to work. The method snapshot() of SnapshotDeletionPolicy was changed to return IndexCommit instead of IndexCommitPoint. IndexCommit used to implement the deprecated IndexCommitPoint, which this patch removes.

      So the tests are compiled against snapshot() returning IndexCommitPoint in the bw-branch, and when run against the method returning IndexCommit of trunk it fails with the exception above.

      Show
      Michael Busch added a comment - Patch for the back-compat trunk. Hmm, everything passes, except this one: [junit] java.lang.NoSuchMethodError: org.apache.lucene.index.SnapshotDeletionPolicy.snapshot()Lorg/apache/lucene/index/IndexCommitPoint; [junit] at org.apache.lucene.TestSnapshotDeletionPolicy.testReuseAcrossWriters(TestSnapshotDeletionPolicy.java:82) [junit] at org.apache.lucene.util.LuceneTestCase.runBare(LuceneTestCase.java:206) [junit] Test org.apache.lucene.TestSnapshotDeletionPolicy FAILED Here drop-in replacement doesn't seem to work. The method snapshot() of SnapshotDeletionPolicy was changed to return IndexCommit instead of IndexCommitPoint. IndexCommit used to implement the deprecated IndexCommitPoint, which this patch removes. So the tests are compiled against snapshot() returning IndexCommitPoint in the bw-branch, and when run against the method returning IndexCommit of trunk it fails with the exception above.
      Hide
      Michael Busch added a comment -

      Same patch as before, but with IndexReader#getFieldCacheKey() undeprecated.

      Is it correct that we keep IndexInput#readChars() and IndexInput#skipChars() for index format compatibility?

      Show
      Michael Busch added a comment - Same patch as before, but with IndexReader#getFieldCacheKey() undeprecated. Is it correct that we keep IndexInput#readChars() and IndexInput#skipChars() for index format compatibility?
      Hide
      Michael McCandless added a comment -

      Is it correct that we keep IndexInput#readChars() and IndexInput#skipChars() for index format compatibility?

      Yes, we need to keep them.

      It was in 2.4 (LUCENE-510) when we switched to writing strings as UTF8, so any index created by eg 2.3 (which we must be able to read through at least 3.9) will need these methods.

      Show
      Michael McCandless added a comment - Is it correct that we keep IndexInput#readChars() and IndexInput#skipChars() for index format compatibility? Yes, we need to keep them. It was in 2.4 ( LUCENE-510 ) when we switched to writing strings as UTF8, so any index created by eg 2.3 (which we must be able to read through at least 3.9) will need these methods.
      Hide
      Michael Busch added a comment -

      OK, thanks for the confirmation. Then this patch removes all deprecations in indexer now.

      I'll wait for comments about the back-compat patch from Uwe first. If I mess up the bw-branch again I'm afraid he'll get mad at me!

      Show
      Michael Busch added a comment - OK, thanks for the confirmation. Then this patch removes all deprecations in indexer now. I'll wait for comments about the back-compat patch from Uwe first. If I mess up the bw-branch again I'm afraid he'll get mad at me!
      Hide
      Uwe Schindler added a comment -

      Hi Michael,

      I see no problem in bw patch One other thing: Can you apply the undeprecations also to 2.9 branch? (Like I did yesterday for the Searchable one).

      Show
      Uwe Schindler added a comment - Hi Michael, I see no problem in bw patch One other thing: Can you apply the undeprecations also to 2.9 branch? (Like I did yesterday for the Searchable one).
      Hide
      Michael Busch added a comment -

      Hi Uwe,

      thanks for taking a look here. However, there is a problem. Please see my previous comment with the exception I'm getting when running test-tag. It seems like I have to commit the change to SnapshotDeletionPolicy#snaphot() also to the bw branch.

      Can you apply the undeprecations also to 2.9 branch?

      Will do!

      Show
      Michael Busch added a comment - Hi Uwe, thanks for taking a look here. However, there is a problem. Please see my previous comment with the exception I'm getting when running test-tag. It seems like I have to commit the change to SnapshotDeletionPolicy#snaphot() also to the bw branch. Can you apply the undeprecations also to 2.9 branch? Will do!
      Hide
      Uwe Schindler added a comment - - edited

      There are two possibilities:

      • Changing the backwards branch main code to change this method
      • remove this test

      I think both possibilities has pros and cons, I tend to the first, because: As you changed the method also in 3.0 trunk, you have to put a backwards incompatibility entry into changes.txt (because we have detected it with the test). In this case it is also valid to change the bw branch code and not only tests.

      (I only looked over the patch, but didn't test it, to find the same things like yesterday. Didn't read all the comments, sorry).

      Show
      Uwe Schindler added a comment - - edited There are two possibilities: Changing the backwards branch main code to change this method remove this test I think both possibilities has pros and cons, I tend to the first, because: As you changed the method also in 3.0 trunk, you have to put a backwards incompatibility entry into changes.txt (because we have detected it with the test). In this case it is also valid to change the bw branch code and not only tests. (I only looked over the patch, but didn't test it, to find the same things like yesterday. Didn't read all the comments, sorry).
      Hide
      Michael Busch added a comment -

      Well IndexCommitPoint was deprecated, so I think this is an expected break - so I don't think an entry in CHANGES.txt is necessary?

      I'm also for the first option. Will do that.

      Show
      Michael Busch added a comment - Well IndexCommitPoint was deprecated, so I think this is an expected break - so I don't think an entry in CHANGES.txt is necessary? I'm also for the first option. Will do that.
      Hide
      Uwe Schindler added a comment -

      I would add the change in CHANGES.txt, too. Its only one line like: "Changed return value of snapshot() from X to Y". Because, if you use it, you cannot do drop in replacements, what you are able to do if you remove all deprecated code from your app in 2.9.

      Show
      Uwe Schindler added a comment - I would add the change in CHANGES.txt, too. Its only one line like: "Changed return value of snapshot() from X to Y". Because, if you use it, you cannot do drop in replacements, what you are able to do if you remove all deprecated code from your app in 2.9.
      Hide
      Michael Busch added a comment -

      Committed revision 825022.

      Show
      Michael Busch added a comment - Committed revision 825022.
      Hide
      Michael Busch added a comment -

      Also added an appropriate entry to the bw breaks section of CHANGES.txt.

      Show
      Michael Busch added a comment - Also added an appropriate entry to the bw breaks section of CHANGES.txt.
      Hide
      Uwe Schindler added a comment -

      I think you missed to delete IndexComitPoint in SVN.

      Show
      Uwe Schindler added a comment - I think you missed to delete IndexComitPoint in SVN.
      Hide
      Uwe Schindler added a comment -

      I think there are some more deprecations (hidden). A search on deprecated finds some more files. E.g.:

      • allowMinus1Position in IndexWriter, DocumentsWriter, DocInverterPerField (field and methods, not correctly marked deprecated).
      • Also IndexReader has still lots of deprecations (e.g. doCommit())
      • CheckIndex#PrintStream
      • some MergePolicy methods
      • IndexWriter#DEFAULT_MERGE_FACTOR, DEFAULT_MAX_MERGE_DOCS, docCount(), abort(), addIndexes()
      Show
      Uwe Schindler added a comment - I think there are some more deprecations (hidden). A search on deprecated finds some more files. E.g.: allowMinus1Position in IndexWriter, DocumentsWriter, DocInverterPerField (field and methods, not correctly marked deprecated). Also IndexReader has still lots of deprecations (e.g. doCommit()) CheckIndex#PrintStream some MergePolicy methods IndexWriter#DEFAULT_MERGE_FACTOR, DEFAULT_MAX_MERGE_DOCS, docCount(), abort(), addIndexes()
      Hide
      Michael Busch added a comment -

      For now I fixed all those that the compiler showed me as warnings.

      I think we should do a text search for "deprecated" on the entire core after the compiler warnings are removed.

      But not now, going to bed...

      Show
      Michael Busch added a comment - For now I fixed all those that the compiler showed me as warnings. I think we should do a text search for "deprecated" on the entire core after the compiler warnings are removed. But not now, going to bed...
      Hide
      Uwe Schindler added a comment -

      javac only shows a warning, if you override a deprecated method and not deprecate it or if you use a deprecated method. But it does not tell you that there are deprecated methods declared.

      I will post a patch with the rest removed.

      Show
      Uwe Schindler added a comment - javac only shows a warning, if you override a deprecated method and not deprecate it or if you use a deprecated method. But it does not tell you that there are deprecated methods declared. I will post a patch with the rest removed.
      Hide
      Uwe Schindler added a comment - - edited

      Here the patch for the rest. It also needs a changed backwards branch. It also adds varargs to some of the addIndexes methods. We should look at other places, too, where to add varargs, too (e.g. IndexSearcher, MultiSearcher,...)

      I have to leave now to Zurich will be back on Friday, but may have some time to look further into it. Michael, can you check the whole patch, I am on a hurry and was not able to test everything (trunk, trunk-contrib, bw).

      Show
      Uwe Schindler added a comment - - edited Here the patch for the rest. It also needs a changed backwards branch. It also adds varargs to some of the addIndexes methods. We should look at other places, too, where to add varargs, too (e.g. IndexSearcher, MultiSearcher,...) I have to leave now to Zurich will be back on Friday, but may have some time to look further into it. Michael, can you check the whole patch, I am on a hurry and was not able to test everything (trunk, trunk-contrib, bw).
      Hide
      Uwe Schindler added a comment -

      My patch has a bug in TestTermVectorAccessor in contrib/misc (rewrite from deprec TermEnum.skip). I do not know whats wrong. Maybe you can fix it, I can look after it later in the evening.

      Show
      Uwe Schindler added a comment - My patch has a bug in TestTermVectorAccessor in contrib/misc (rewrite from deprec TermEnum.skip). I do not know whats wrong. Maybe you can fix it, I can look after it later in the evening.
      Hide
      Michael Busch added a comment -

      I just ran all tests. test-contrib and test-tag both fail for me. I can look into it a bit later today...

      Show
      Michael Busch added a comment - I just ran all tests. test-contrib and test-tag both fail for me. I can look into it a bit later today...
      Hide
      Michael Busch added a comment -

      This one fails in the bw-branch: org.apache.lucene.index.TestThreadedOptimize

      Show
      Michael Busch added a comment - This one fails in the bw-branch: org.apache.lucene.index.TestThreadedOptimize
      Hide
      Michael Busch added a comment -

      Fixes the bug in TermVectorAccessor.

      Show
      Michael Busch added a comment - Fixes the bug in TermVectorAccessor.
      Hide
      Michael Busch added a comment -

      And the fix for the bw-branch.

      Running all tests again now...

      Show
      Michael Busch added a comment - And the fix for the bw-branch. Running all tests again now...
      Hide
      Uwe Schindler added a comment - - edited

      Thanks!
      Your patch for TermVectorAccessor is fine, only the termEnum.close() should also be inside the if-branch.

      EDIT: sorry, my fault.

      Show
      Uwe Schindler added a comment - - edited Thanks! Your patch for TermVectorAccessor is fine, only the termEnum.close() should also be inside the if-branch. EDIT: sorry, my fault.
      Hide
      Uwe Schindler added a comment -

      Seems that in the bw branch one more docCount() was in the tests... Sorry. I just patched the bw branch with the trunk test patch

      Running tests now, too.

      Show
      Uwe Schindler added a comment - Seems that in the bw branch one more docCount() was in the tests... Sorry. I just patched the bw branch with the trunk test patch Running tests now, too.
      Hide
      Michael Busch added a comment -

      Seems that in the bw branch one more docCount() was in the tests

      Yeah I just removed the line, because the variable wasn't actually used anywhere.

      OK all tests passed (core, contrib, bw).
      +1 for committing.

      Show
      Michael Busch added a comment - Seems that in the bw branch one more docCount() was in the tests Yeah I just removed the line, because the variable wasn't actually used anywhere. OK all tests passed (core, contrib, bw). +1 for committing.
      Hide
      Uwe Schindler added a comment -

      +1 all tests pass here, too!

      Show
      Uwe Schindler added a comment - +1 all tests pass here, too!
      Hide
      Michael Busch added a comment -

      Shall I commit it or are you going to, Uwe? Just asking, so that we're not trying to do it at the same time...

      Show
      Michael Busch added a comment - Shall I commit it or are you going to, Uwe? Just asking, so that we're not trying to do it at the same time...
      Hide
      Uwe Schindler added a comment -

      do it!

      Show
      Uwe Schindler added a comment - do it!
      Hide
      Michael Busch added a comment -

      Committed revision 825288.

      Thanks, Uwe!

      Show
      Michael Busch added a comment - Committed revision 825288. Thanks, Uwe!

        People

        • Assignee:
          Unassigned
          Reporter:
          Michael Busch
        • Votes:
          0 Vote for this issue
          Watchers:
          0 Start watching this issue

          Dates

          • Created:
            Updated:
            Resolved:

            Development