Lucene - Core
  1. Lucene - Core
  2. LUCENE-5946

Change SimpleFSDirectory to use newByteChannel

    Details

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

      Description

      Currently our javadocs point to using SimpleFSDirectory "to avoid ClosedByInterruptException".

      But this is really bogus. If you interrupt() a thread doing i/o, then you need to be prepared for the consequences. Its just that RAF is broken and not interruptible at all, but that shouldnt justify us continuing to use it.

      SimpleFSDirectory should be "the most portable", so it should use Files.newByteChannel. And we should remove the javadocs/warnings about ClosedByInterrupt

        Activity

        Hide
        Uwe Schindler added a comment -

        One possibility would be to move the old SimpleFSDirectory to lucene/misc or lucene/snapshot, named RAFDirectory. SimpleFSDirectory in trunk should use Java 7 APIs only. The interrupt() arguments were never seen in any issue report, it was only Simon mentioning it on an issue.

        Just dont use Future.cancel(true): http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Future.html#cancel(boolean)

        It's your own fault if you do this with Lucene code.

        Show
        Uwe Schindler added a comment - One possibility would be to move the old SimpleFSDirectory to lucene/misc or lucene/snapshot, named RAFDirectory. SimpleFSDirectory in trunk should use Java 7 APIs only. The interrupt() arguments were never seen in any issue report, it was only Simon mentioning it on an issue. Just dont use Future.cancel(true): http://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Future.html#cancel(boolean) It's your own fault if you do this with Lucene code.
        Hide
        Robert Muir added a comment -

        Here's a patch with Uwe's approach. I put the current RandomAccessFile one in misc/ as RAFDirectory and cutover SimpleFSDirectory.

        all tests pass.

        Show
        Robert Muir added a comment - Here's a patch with Uwe's approach. I put the current RandomAccessFile one in misc/ as RAFDirectory and cutover SimpleFSDirectory. all tests pass.
        Hide
        Uwe Schindler added a comment -

        Looks good! Maybe improve the documentation of the contrib/misc RAFDirectory to explain why it actually exists!

        By the way: why did NIO use 16384 bytes bufsize instead of the default 8192? In my opinion we should consequently use 8 KiB, especially if we do heavy random access like docvalues.

        But this is another discussion for another issue.

        Show
        Uwe Schindler added a comment - Looks good! Maybe improve the documentation of the contrib/misc RAFDirectory to explain why it actually exists! By the way: why did NIO use 16384 bytes bufsize instead of the default 8192? In my opinion we should consequently use 8 KiB, especially if we do heavy random access like docvalues. But this is another discussion for another issue.
        Hide
        Uwe Schindler added a comment - - edited

        By the way: why did NIO use 16384 bytes bufsize instead of the default 8192? In my opinion we should consequently use 8 KiB, especially if we do heavy random access like docvalues.
        But this is another discussion for another issue.

        We no longer use RAF here, so the malloc "bug" in the JVM does not apply. So 16384 is a good choice here, too (was extensively tested for NIOFSDir)! We should just not make the buffer larger, because NIO internally has a pool of DirectBuffers to handle the actual disk I/O.

        In addition, this is unrelated to the actual buffer size. This is just the maximum to read as chunk if some code requests megabytes of data. The BufferedIndexInput buffering is unrelated, so most reads (like docvalues) use much smaller buffers.

        Show
        Uwe Schindler added a comment - - edited By the way: why did NIO use 16384 bytes bufsize instead of the default 8192? In my opinion we should consequently use 8 KiB, especially if we do heavy random access like docvalues. But this is another discussion for another issue. We no longer use RAF here, so the malloc "bug" in the JVM does not apply. So 16384 is a good choice here, too (was extensively tested for NIOFSDir)! We should just not make the buffer larger, because NIO internally has a pool of DirectBuffers to handle the actual disk I/O. In addition, this is unrelated to the actual buffer size. This is just the maximum to read as chunk if some code requests megabytes of data. The BufferedIndexInput buffering is unrelated, so most reads (like docvalues) use much smaller buffers.
        Hide
        Robert Muir added a comment -

        Thanks Uwe, I will add the following note to it when committing:

        /**
         *  ...
         *  NOTE: Because this uses RandomAccessFile, it will generally
         *  not work with non-default filesystem providers. It is only
         *  provided for applications that relied on the fact that 
         *  RandomAccessFile's IO was not interruptible.
         */
        
        Show
        Robert Muir added a comment - Thanks Uwe, I will add the following note to it when committing: /** * ... * NOTE: Because this uses RandomAccessFile, it will generally * not work with non- default filesystem providers. It is only * provided for applications that relied on the fact that * RandomAccessFile's IO was not interruptible. */
        Hide
        Uwe Schindler added a comment -

        Thanks, looks fine!

        Show
        Uwe Schindler added a comment - Thanks, looks fine!
        Hide
        ASF subversion and git services added a comment -

        Commit 1624879 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1624879 ]

        LUCENE-5946: fix incorrect javadocs link

        Show
        ASF subversion and git services added a comment - Commit 1624879 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1624879 ] LUCENE-5946 : fix incorrect javadocs link
        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:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development