Lucene - Core
  1. Lucene - Core
  2. LUCENE-1672

Deprecate all String/File ctors/opens in IndexReader/IndexWriter/IndexSearcher

    Details

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

      Description

      During investigation of LUCENE-1658, I found out, that even LUCENE-1453 is not completely fixed.
      As 1658 deprecates all FSDirectory.getDirectory() static factories, we should not use them anymore. As the user is now free to choose the correct directory implementation using direct instantiation or using FSDir.open() he should no longer use all ctors/methods in IndexWriter/IndexReader/IndexSearcher & Co. that simply take path names as String or File and always instantiate the Directory himself.

      LUCENE-1453 currently works for the cached directory implementations from FSDir.getDirectory, but not with uncached, non refcounting FSDirs. Sometime reopen() closes the directory (as far as I see, when a SegmentReader changes to a MultiSegmentReader and/or deletes apply). This is hard to track. In Lucene 3.0 we then can remove the whole bunch of closeDirectory parameters/fields in these classes and simply do not care anymore about closing directories.

      To remove this closeDirectory parameter now (before 3.0) and also fix 1453 correctly, an additional idea would be to change these factories that take the File/String to return the IndexReader wrapped by a FilteredIndexReader, that keeps track on closing the underlying directory after close and reopen. This is simplier than passing this boolean between different DirectoryIndexReader instances. The small performance impact by wrapping with FilterIndexReader should not be so bad, because the method is deprecated and we can state, that it is better to user the factory method with Directory parameter.

      1. LUCENE-1672.patch
        28 kB
        Uwe Schindler
      2. LUCENE-1672.patch
        23 kB
        Uwe Schindler
      3. LUCENE-1672.patch
        28 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Shai Erera added a comment -

          Can we stick with just a File argument, no String?

          Show
          Shai Erera added a comment - Can we stick with just a File argument, no String?
          Hide
          Uwe Schindler added a comment -

          But the File argument has the same problem with passing all these closeDirectory arguments around. If we wrap this using this FilterIndexReader, ok, but as discussed yesterday, I would prefer to simply only allow Directory as input parameter and leave specifying the dir impl to the user. Automatically use FSDir.open() is also not the best choose, because then user ask again of java-user (how to specify another impl and so on).
          For the user it is just a little bit more code and the additional close() call (which can normally left out for standard directories...)

          Show
          Uwe Schindler added a comment - But the File argument has the same problem with passing all these closeDirectory arguments around. If we wrap this using this FilterIndexReader, ok, but as discussed yesterday, I would prefer to simply only allow Directory as input parameter and leave specifying the dir impl to the user. Automatically use FSDir.open() is also not the best choose, because then user ask again of java-user (how to specify another impl and so on). For the user it is just a little bit more code and the additional close() call (which can normally left out for standard directories...)
          Hide
          Shai Erera added a comment -

          Ooops, my fault. I didn't read the subject and description carefully.

          Yep, sticking w/ Dir ctorsshould definitely simplify things.

          Show
          Shai Erera added a comment - Ooops, my fault. I didn't read the subject and description carefully. Yep, sticking w/ Dir ctorsshould definitely simplify things.
          Hide
          Earwin Burrfoot added a comment -

          Yahoo! I was going to create the same issue

          Show
          Earwin Burrfoot added a comment - Yahoo! I was going to create the same issue
          Hide
          Uwe Schindler added a comment -

          Here is a first patch, deprecating all usages of File/String to specify a directory. I am not sure, if I hit all, if somebody knows another one, tell it.

          This patch also replaces FSDirectory.getDirectory() by FSDirectory.open() for all non-deprecated methods. The deprecated IndexReader/IndexWriter and so on methods still use FSDir.getDirectory because they sometimes depend on refCounting (which is wrong). This is a bug related to LUCENE-1453.

          I will post a later patch that fixes this by wrapping all IndexReaders instantiated by File/String with a FilterIndexReader that manages the closing of the underlying directory. All closeDirectory/closeDir pass-throughs everywhere can then be removed.

          This also fixes a small bug in IndexModifier to close the directory on close() when dir was given as File/String.

          I also removed File/String in the segment file finder (non-public API).

          Show
          Uwe Schindler added a comment - Here is a first patch, deprecating all usages of File/String to specify a directory. I am not sure, if I hit all, if somebody knows another one, tell it. This patch also replaces FSDirectory.getDirectory() by FSDirectory.open() for all non-deprecated methods. The deprecated IndexReader/IndexWriter and so on methods still use FSDir.getDirectory because they sometimes depend on refCounting (which is wrong). This is a bug related to LUCENE-1453 . I will post a later patch that fixes this by wrapping all IndexReaders instantiated by File/String with a FilterIndexReader that manages the closing of the underlying directory. All closeDirectory/closeDir pass-throughs everywhere can then be removed. This also fixes a small bug in IndexModifier to close the directory on close() when dir was given as File/String. I also removed File/String in the segment file finder (non-public API).
          Hide
          Uwe Schindler added a comment -

          Updated patch merged with Mike's last commit.

          Show
          Uwe Schindler added a comment - Updated patch merged with Mike's last commit.
          Hide
          Michael McCandless added a comment -

          Patch looks good Uwe! You don't really need to deprecate methods in IndexModifier, since the whole class is deprecated.

          Show
          Michael McCandless added a comment - Patch looks good Uwe! You don't really need to deprecate methods in IndexModifier, since the whole class is deprecated.
          Hide
          Uwe Schindler added a comment - - edited

          With IndexModifier, you are right. I was just adding this closeDir stuff (which is missing here and is a real bug) and deprecated these methods. In the JavaDocs every method is deprecated automatically, so I do not need to do it specifically.

          I will later try to solve this problem with the closeDir inside the different IndexReaders (but maybe Earwin has done it already in LUCENE-1651) with a filtered IndexReader returned by the open() methods taking a String/File. I can then revert/remove all these closeDir parameters/members.

          Show
          Uwe Schindler added a comment - - edited With IndexModifier, you are right. I was just adding this closeDir stuff (which is missing here and is a real bug) and deprecated these methods. In the JavaDocs every method is deprecated automatically, so I do not need to do it specifically. I will later try to solve this problem with the closeDir inside the different IndexReaders (but maybe Earwin has done it already in LUCENE-1651 ) with a filtered IndexReader returned by the open() methods taking a String/File. I can then revert/remove all these closeDir parameters/members.
          Hide
          Earwin Burrfoot added a comment -

          I will later try to solve this problem with the closeDir inside the different IndexReaders (but maybe Earwin has done it already in LUCENE-1651)

          My issue removes closeDir from SegmentReader, as it cannot 'own' a directory anymore. MSR-to-be-DirectoryReader still has this flag.

          Show
          Earwin Burrfoot added a comment - I will later try to solve this problem with the closeDir inside the different IndexReaders (but maybe Earwin has done it already in LUCENE-1651 ) My issue removes closeDir from SegmentReader, as it cannot 'own' a directory anymore. MSR-to-be-DirectoryReader still has this flag.
          Hide
          Uwe Schindler added a comment -

          Nice. And DirectoryIR/MSR still have this Flag, but reopening a MSR always returns a MSR again (even if it only consists of one segment)?
          If this is so, I wait for your issue to be closed and then rework here.

          To test, if the closedir (LUCENE-1453) issue is correctly solved, try to replace FSDir.getDirectory by FSDir.open(). in IndexReader.open() taking File/String. If TestIndexReaderReopen then passes (even with different random seeds, so run test multiple times) always, it is really fixed.

          Show
          Uwe Schindler added a comment - Nice. And DirectoryIR/MSR still have this Flag, but reopening a MSR always returns a MSR again (even if it only consists of one segment)? If this is so, I wait for your issue to be closed and then rework here. To test, if the closedir ( LUCENE-1453 ) issue is correctly solved, try to replace FSDir.getDirectory by FSDir.open(). in IndexReader.open() taking File/String. If TestIndexReaderReopen then passes (even with different random seeds, so run test multiple times) always, it is really fixed.
          Hide
          Earwin Burrfoot added a comment -

          And DirectoryIR/MSR still have this Flag, but reopening a MSR always returns a MSR again (even if it only consists of one segment)?

          Exactly.

          Show
          Earwin Burrfoot added a comment - And DirectoryIR/MSR still have this Flag, but reopening a MSR always returns a MSR again (even if it only consists of one segment)? Exactly.
          Hide
          Uwe Schindler added a comment -

          Mike: Thanks for committing LUCENE-1651!
          I will update this patch and commit sometime this evening.
          Before I do this, I will also check, if now reopen() works correctly when replacing all FSDir.getDir() by FSDir.open().

          Show
          Uwe Schindler added a comment - Mike: Thanks for committing LUCENE-1651 ! I will update this patch and commit sometime this evening. Before I do this, I will also check, if now reopen() works correctly when replacing all FSDir.getDir() by FSDir.open().
          Hide
          Michael McCandless added a comment -

          Excellent!

          Show
          Michael McCandless added a comment - Excellent!
          Hide
          Uwe Schindler added a comment -

          Updated patch. I commit shortly and close this issue.

          Show
          Uwe Schindler added a comment - Updated patch. I commit shortly and close this issue.
          Hide
          Uwe Schindler added a comment -

          Fixed revision 782469.

          Show
          Uwe Schindler added a comment - Fixed revision 782469.

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development