Lucene - Core
  1. Lucene - Core
  2. LUCENE-4982

Make MockIndexOutputWrapper check disk full on copyBytes

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.4, Trunk
    • Component/s: modules/test-framework
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      While working on the consistency test for Replicator (LUCENE-4975), I noticed that I don't trip disk-full exceptions and tracked it down to MockIndexOutputWrapper.copyBytes not doing these checks like writeBytes. I'd like to add this check.

      1. LUCENE-4982.patch
        6 kB
        Shai Erera
      2. LUCENE-4982.patch
        6 kB
        Shai Erera
      3. LUCENE-4982.patch
        6 kB
        Shai Erera

        Activity

        Steve Rowe made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Hide
        Steve Rowe added a comment -

        Bulk close resolved 4.4 issues

        Show
        Steve Rowe added a comment - Bulk close resolved 4.4 issues
        Shai Erera made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 5.0 [ 12321663 ]
        Fix Version/s 4.4 [ 12324323 ]
        Resolution Fixed [ 1 ]
        Hide
        Shai Erera added a comment -

        Committed to trunk and 4x

        Show
        Shai Erera added a comment - Committed to trunk and 4x
        Hide
        Shai Erera added a comment -

        I thought about this some more and I realize that getComputedActualSizeInBytes works as expected. checkDiskFull should only trip if the Directory size has reached the limit, and it cannot tell how many bytes are pending in a buffer. The test would fail not only w/ RAMDirectory, but also a Directory which buffers writes (which I believe all our directories do), and therefore flush() is important for the test.

        So to summarize the changes in this issue:

        • Added checkDiskFull to MockIOWrapper so it can trip writeBytes and copyBytes.
        • Changed checkDiskFull to do freeSpace < len because freeSpace == len is still valid.
        • Added a test

        I plan to commit this tomorrow.

        Show
        Shai Erera added a comment - I thought about this some more and I realize that getComputedActualSizeInBytes works as expected. checkDiskFull should only trip if the Directory size has reached the limit, and it cannot tell how many bytes are pending in a buffer. The test would fail not only w/ RAMDirectory, but also a Directory which buffers writes (which I believe all our directories do), and therefore flush() is important for the test. So to summarize the changes in this issue: Added checkDiskFull to MockIOWrapper so it can trip writeBytes and copyBytes. Changed checkDiskFull to do freeSpace < len because freeSpace == len is still valid. Added a test I plan to commit this tomorrow.
        Shai Erera made changes -
        Attachment LUCENE-4982.patch [ 12581895 ]
        Hide
        Shai Erera added a comment - - edited

        I changed the check to freeSpace < len, but then the test failed to trip disk-full the second time, unless I call out.flush() in between. Debugging tells me that RAMOutputStream sets RAMFile.length only on flush(), therefore even if I attempt to write a 2K byte[] (with maxSize=2), the test doesn't fail.

        Seems like getRecomputedActualSizeInBytes is not very accurate. It only returns the size of the flushed files (even for FSDir). This may be ok, dunno. It just felt wrong for RAMDirectory, since there is no real buffering happening.

        Anyway, I guess we'll have to live with that. Disk-full is anyway a best effort, so in this test, I'll just call flush(). In real tests that want to trip disk-full, usually indexing happens and therefore files get flushed, and the size measure is closer.

        Show
        Shai Erera added a comment - - edited I changed the check to freeSpace < len , but then the test failed to trip disk-full the second time, unless I call out.flush() in between. Debugging tells me that RAMOutputStream sets RAMFile.length only on flush(), therefore even if I attempt to write a 2K byte[] (with maxSize=2), the test doesn't fail. Seems like getRecomputedActualSizeInBytes is not very accurate. It only returns the size of the flushed files (even for FSDir). This may be ok, dunno. It just felt wrong for RAMDirectory, since there is no real buffering happening. Anyway, I guess we'll have to live with that. Disk-full is anyway a best effort, so in this test, I'll just call flush(). In real tests that want to trip disk-full, usually indexing happens and therefore files get flushed, and the size measure is closer.
        Shai Erera made changes -
        Attachment LUCENE-4982.patch [ 12581881 ]
        Hide
        Shai Erera added a comment -

        I modified the test to set maxSize=2 and then write 2 bytes in two calls. The first should succeed, the second fail. However, even the first fails and now I don't know if it's a bug in the test or MockIO.checkDiskFull(). The latter (copy of the original code) does freeSpace <= len – is this ok? I mean, if I have room for 2 bytes and the caller asks to write 2 bytes, should we really fail on diskFull?

        Show
        Shai Erera added a comment - I modified the test to set maxSize=2 and then write 2 bytes in two calls. The first should succeed, the second fail. However, even the first fails and now I don't know if it's a bug in the test or MockIO.checkDiskFull(). The latter (copy of the original code) does freeSpace <= len – is this ok? I mean, if I have room for 2 bytes and the caller asks to write 2 bytes, should we really fail on diskFull?
        Hide
        Shai Erera added a comment -

        I can modify the test sure. But the problem is that copyBytes doesn't call writeBytes, otherwise I would have tripped it. I.e., we call delegate.copyBytes, which internally may call its writeBytes, but not MockIO.writeBytes.

        Show
        Shai Erera added a comment - I can modify the test sure. But the problem is that copyBytes doesn't call writeBytes, otherwise I would have tripped it. I.e., we call delegate.copyBytes, which internally may call its writeBytes, but not MockIO.writeBytes.
        Hide
        Robert Muir added a comment -

        Its not clear to me if with the patch we will double-count against disk full if copyBytes calls writeBytes behind the scenes...

        Maybe we can make the test have a max size of 2 bytes and copyBytes twice to it just so this is obvious?

        Show
        Robert Muir added a comment - Its not clear to me if with the patch we will double-count against disk full if copyBytes calls writeBytes behind the scenes... Maybe we can make the test have a max size of 2 bytes and copyBytes twice to it just so this is obvious?
        Hide
        Michael McCandless added a comment -

        +1, good catch.

        Who tests the tester!

        Show
        Michael McCandless added a comment - +1, good catch. Who tests the tester!
        Shai Erera made changes -
        Attachment LUCENE-4982.patch [ 12581860 ]
        Hide
        Shai Erera added a comment -

        Patch adds a test to TestMockDirWrapper and factors out checkDiskFull method in MockIOWrapper. The signature is a bit ugly, but that's needed because checkDiskFull copies the remaining bytes, and writeBytes copies from an array while copyBytes from DataInput. I don't think it's the end of the world, but if anyone has an idea how to do it better...

        I ran core tests and they passed (actually only 3 tests under core set dir.maxSize).

        Show
        Shai Erera added a comment - Patch adds a test to TestMockDirWrapper and factors out checkDiskFull method in MockIOWrapper. The signature is a bit ugly, but that's needed because checkDiskFull copies the remaining bytes, and writeBytes copies from an array while copyBytes from DataInput. I don't think it's the end of the world, but if anyone has an idea how to do it better... I ran core tests and they passed (actually only 3 tests under core set dir.maxSize).
        Shai Erera made changes -
        Field Original Value New Value
        Component/s modules/test-framework [ 12320591 ]
        Component/s general/test [ 12313730 ]
        Shai Erera created issue -

          People

          • Assignee:
            Shai Erera
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development