Lucene - Core
  1. Lucene - Core
  2. LUCENE-773

Deprecate "create" method in FSDirectory.getDirectory in favor of IndexWriter's "create"

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0.0, 2.1
    • Fix Version/s: 2.1
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      It's confusing that there is a create=true|false at the FSDirectory
      level and then also another create=true|false at the IndexWriter
      level. Which one should you use when creating an index?

      Our users have been confused by this in the past:

      http://www.gossamer-threads.com/lists/lucene/java-user/4792

      I think in general we should try to have one obvious way to achieve
      something (like Python: http://en.wikipedia.org/wiki/Python_philosophy).

      And the fact that there are now two code paths that are supposed to do
      the same (similar?) thing, can more easily lead to sneaky bugs. One
      case of LUCENE-140 (already fixed in trunk but not past releases),
      which inspired this issue, can happen if you send create=false to the
      FSDirectory and create=true to the IndexWriter.

      Finally, as of lockless commits, it is now possible to open an
      existing index for "create" while readers are still using the old
      "point in time" index, on Windows. (At least one user had tried this
      previously and failed). To do this, we use the IndexFileDeleter class
      (which retries on failure) and we also look at the segments file to
      determine the next segments_N file to write to.

      With future issues like LUCENE-710 even more "smarts" may be required
      to know what it takes to "create" a new index into an existing
      directory. Given that we have have quite a few Directory
      implemenations, I think these "smarts" logically should live in
      IndexWriter (not replicated in each Directory implementation), and we
      should leave the Directory as an interface that knows how to make
      changes to some backing store but does not itself try to make any
      changes.

        Activity

        Hide
        Michael McCandless added a comment -

        Closing all issues that were resolved for 2.1.

        Show
        Michael McCandless added a comment - Closing all issues that were resolved for 2.1.
        Hide
        Michael McCandless added a comment -

        OK I added a unit test that hits the NPE, and corrected it so the test
        now passes.

        Sorry about this and thanks for catching it Aaron!

        Show
        Michael McCandless added a comment - OK I added a unit test that hits the NPE, and corrected it so the test now passes. Sorry about this and thanks for catching it Aaron!
        Hide
        Michael McCandless added a comment -

        My fix here broke one case of backwards compatibility for Directory implementations that provide their own locking implementation (ie, that do not use Lockfactory). See here:

        http://www.gossamer-threads.com/lists/lucene/java-dev/44555

        Show
        Michael McCandless added a comment - My fix here broke one case of backwards compatibility for Directory implementations that provide their own locking implementation (ie, that do not use Lockfactory). See here: http://www.gossamer-threads.com/lists/lucene/java-dev/44555
        Hide
        Michael McCandless added a comment -

        OK I committed this:

        • Added removal of old write lock in IndexWriter's create (it
          already removes unreferenced files).
        • Deprecated FSDirectory.getDirectory constructors that take a
          boolean create.
        • Verified all unit tests still pass while using deprecated create.
        • Fixed all but one unit test to no longer use the now-deprecated
          constructors (I left one to make sure we do in fact continue to
          test the deprecated getDirectory constructor). I added an "rmDir"
          utility method in a new class o/a/l/util/_TestUtil.java for those
          tests that were using getDirectory to just remove a directory.
        • Removed FSDirectory.getDirectory methods that had "create" that we
          had added with LockFactory. These methods were never released so
          no sense newly releasing deprecated APIs.
        Show
        Michael McCandless added a comment - OK I committed this: Added removal of old write lock in IndexWriter's create (it already removes unreferenced files). Deprecated FSDirectory.getDirectory constructors that take a boolean create. Verified all unit tests still pass while using deprecated create. Fixed all but one unit test to no longer use the now-deprecated constructors (I left one to make sure we do in fact continue to test the deprecated getDirectory constructor). I added an "rmDir" utility method in a new class o/a/l/util/_TestUtil.java for those tests that were using getDirectory to just remove a directory. Removed FSDirectory.getDirectory methods that had "create" that we had added with LockFactory. These methods were never released so no sense newly releasing deprecated APIs.
        Hide
        Nicolas Lalevée added a comment -

        ho, yeah, it's the 662 of course, my eyes might have squited. (and it can't be the 622 as I am definitively not a maven user ^^)

        Show
        Nicolas Lalevée added a comment - ho, yeah, it's the 662 of course, my eyes might have squited. (and it can't be the 622 as I am definitively not a maven user ^^)
        Hide
        Michael McCandless added a comment -

        Nicolas, I think you meant LUCENE-662 above?

        But I'll take the +1

        Show
        Michael McCandless added a comment - Nicolas, I think you meant LUCENE-662 above? But I'll take the +1
        Hide
        Nicolas Lalevée added a comment -

        forget what I have said about "removing any index structure specificity in the store package.". Actually, the directory is the only central instance that can holds an indexformat instance.

        Anyway, I still +1 for not duplicating code !

        Show
        Nicolas Lalevée added a comment - forget what I have said about "removing any index structure specificity in the store package.". Actually, the directory is the only central instance that can holds an indexformat instance. Anyway, I still +1 for not duplicating code !
        Hide
        Nicolas Lalevée added a comment -

        I was working on the IndexFormat mechanism, LUCENE-622 being the first draft of it. And I have tried to use some Java-1.5 parametered types to see if it is possible to make index readers/writers typed by the index format. And I am in front of one issue : the directory have to know the index format because of the IndexNameFilter and the create feature. And I don't think it is a good idea because of how they are instanciated.

        I have not finished the design of this Java-1.5-way-of-typing, I have other issues to look at, but I vote +1 for removing any index structure specificity in the store package.

        Show
        Nicolas Lalevée added a comment - I was working on the IndexFormat mechanism, LUCENE-622 being the first draft of it. And I have tried to use some Java-1.5 parametered types to see if it is possible to make index readers/writers typed by the index format. And I am in front of one issue : the directory have to know the index format because of the IndexNameFilter and the create feature. And I don't think it is a good idea because of how they are instanciated. I have not finished the design of this Java-1.5-way-of-typing, I have other issues to look at, but I vote +1 for removing any index structure specificity in the store package.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development