Uploaded image for project: 'Cassandra'
  1. Cassandra
  2. CASSANDRA-4050

Rewrite RandomAccessReader to use FileChannel / nio to address Windows file access violations

    Details

      Description

      On Windows w/older java I/O libraries the files are not opened with FILE_SHARE_DELETE. This causes problems as hard-links cannot be deleted while the original file is opened - our snapshots are a big problem in particular. The nio library and FileChannels open with FILE_SHARE_DELETE which should help remedy this problem.

      Original text:
      I'm using Cassandra 1.0.8, on Windows 7. When I take a snapshot of the database, I find that I am unable to delete the snapshot directory (i.e., dir named "

      {datadir}

      {keyspacename}\snapshots{snapshottag}") while Cassandra is running: "The action can't be completed because the folder or a file in it is open in another program. Close the folder or file and try again" [in Windows Explorer]. If I terminate Cassandra, then I can delete the directory with no problem.

      I expect to be able to move or delete the snapshotted files while Cassandra is running, as this should not affect the runtime operation of Cassandra.

      1. CASSANDRA-4050_v1.patch
        28 kB
        Joshua McKenzie
      2. CASSANDRA-4050_v2.patch
        29 kB
        Joshua McKenzie
      3. CASSANDRA-4050_v3.patch
        29 kB
        Joshua McKenzie

        Issue Links

          Activity

          Hide
          jbellis Jonathan Ellis added a comment -

          Currently we take the snapshot using mklink /H, but I've experimented with the Java7 Files.createLink and see the same behavior. It may simply be normal behavior for Windows that links are considered "open" until their creator is closed.

          Show
          jbellis Jonathan Ellis added a comment - Currently we take the snapshot using mklink /H, but I've experimented with the Java7 Files.createLink and see the same behavior. It may simply be normal behavior for Windows that links are considered "open" until their creator is closed.
          Hide
          jn Jim Newsham added a comment -

          Yeah I'm starting to think that might be true – unfortunately. I haven't found anything definitive, but the following postings imply a hardlink cannot be deleted while another hardlink to the same file is locked:

          http://superuser.com/questions/387136/is-it-possible-to-delete-a-hardlink-to-a-locked-file
          http://superuser.com/questions/301303/one-hardlink-is-locked-how-do-i-remove-the-other

          Perhaps the data structure that records the lock is in the file object and not the hardlink object.

          Show
          jn Jim Newsham added a comment - Yeah I'm starting to think that might be true – unfortunately. I haven't found anything definitive, but the following postings imply a hardlink cannot be deleted while another hardlink to the same file is locked: http://superuser.com/questions/387136/is-it-possible-to-delete-a-hardlink-to-a-locked-file http://superuser.com/questions/301303/one-hardlink-is-locked-how-do-i-remove-the-other Perhaps the data structure that records the lock is in the file object and not the hardlink object.
          Hide
          akashirin Alexander Kashirin added a comment -

          I have the same problem with Cassandra 1.1.5 on Windows 7 and Windows Server 2008 R2:

          The nodetool stably creates snapshots that can't be deleted by the "clearsnapshot" command later (- IOException: Failed to delete ...). The bare facts are below.

          For some (not for all) snapshot files (= hard links):

          1) WinExplorer->Delete says "The process cannot access the file because it is being used by another process".
          2) Command interpreter writes "Access denied" on "del" command.
          3) Troubleshooting tools (- "Process Explorer" and "Unlocker") do not find open file handles. Moreover, Unlocker is able to delete these files without problems.
          4) The files become deletable by the rest tools just after Cassandra-server has been stopped.
          5) After restart, the same files become locked again.
          6) Everything repeats after computer has been restarted.

          Show
          akashirin Alexander Kashirin added a comment - I have the same problem with Cassandra 1.1.5 on Windows 7 and Windows Server 2008 R2: The nodetool stably creates snapshots that can't be deleted by the "clearsnapshot" command later (- IOException: Failed to delete ...). The bare facts are below. For some (not for all) snapshot files (= hard links): 1) WinExplorer->Delete says "The process cannot access the file because it is being used by another process". 2) Command interpreter writes "Access denied" on "del" command. 3) Troubleshooting tools (- "Process Explorer" and "Unlocker") do not find open file handles. Moreover, Unlocker is able to delete these files without problems. 4) The files become deletable by the rest tools just after Cassandra-server has been stopped. 5) After restart, the same files become locked again. 6) Everything repeats after computer has been restarted.
          Hide
          jbellis Jonathan Ellis added a comment -

          Right, so NTFS is locking the underlying data since the "original" sstable is still open in Cassandra, so the behavior described by Jim applies. We'd have to add a workaround like the one given on superuser.com – move the links to a "garbage" location to clean up on restart.

          This is pretty low priority for me but I'd be glad to point someone interested in the right direction.

          Show
          jbellis Jonathan Ellis added a comment - Right, so NTFS is locking the underlying data since the "original" sstable is still open in Cassandra, so the behavior described by Jim applies. We'd have to add a workaround like the one given on superuser.com – move the links to a "garbage" location to clean up on restart. This is pretty low priority for me but I'd be glad to point someone interested in the right direction.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          There's been some discussion on CASSANDRA-6283 concerning this. From the comments:

          With our current io implementation on Windows, snapshots won't be deletable as long as the original sstable is locked. It's going to take a new FileDataInput based on FileChannel w/jdk7 using the FILE_SHARE_DELETE flag to allow deletion of hard links while the original file is open.

          Bug with behavior: http://bugs.java.com/view_bug.do?bug_id=6607535
          jdk7 support: http://www.docjar.com/html/api/sun/nio/fs/WindowsChannelFactory.java.html

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - There's been some discussion on CASSANDRA-6283 concerning this. From the comments: With our current io implementation on Windows, snapshots won't be deletable as long as the original sstable is locked. It's going to take a new FileDataInput based on FileChannel w/jdk7 using the FILE_SHARE_DELETE flag to allow deletion of hard links while the original file is open. Bug with behavior: http://bugs.java.com/view_bug.do?bug_id=6607535 jdk7 support: http://www.docjar.com/html/api/sun/nio/fs/WindowsChannelFactory.java.html
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited

          I did some testing to confirm - nio.2 resolves both deleting hard-links and deleting of files with other handles currently open on Windows. It should be straightforward to convert to a FileChannel and ByteBuffer in RAR and wrap the FileDataInput interface over to the ByteBuffer's.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited I did some testing to confirm - nio.2 resolves both deleting hard-links and deleting of files with other handles currently open on Windows. It should be straightforward to convert to a FileChannel and ByteBuffer in RAR and wrap the FileDataInput interface over to the ByteBuffer's.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          After changing RAF over to nio.2-based I'm still seeing snapshot deletion errors. A little digging turned up:
          http://bugs.java.com/view_bug.do?bug_id=4715154

          As this applies to deleting memory mapped files but not necessarily hard-links with memory mapped segments in the original I figured I'd test it. I've confirmed that if the original file has any data that's in a MappedByteBuffer even if the original RAF it was associated with is closed, Windows refuses to delete the hard-link.

          Some local logging indicates that we have MmappedSegmentedFile Segments open on the original keyspaces in question when deletion is attempted which isn't surprising. The following implies that there might be a way around this but it involves turning off all page caching for these files which isn't what we want for SSTableReaders on Windows: http://www.osronline.com/showThread.cfm?link=64732

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - After changing RAF over to nio.2-based I'm still seeing snapshot deletion errors. A little digging turned up: http://bugs.java.com/view_bug.do?bug_id=4715154 As this applies to deleting memory mapped files but not necessarily hard-links with memory mapped segments in the original I figured I'd test it. I've confirmed that if the original file has any data that's in a MappedByteBuffer even if the original RAF it was associated with is closed, Windows refuses to delete the hard-link. Some local logging indicates that we have MmappedSegmentedFile Segments open on the original keyspaces in question when deletion is attempted which isn't surprising. The following implies that there might be a way around this but it involves turning off all page caching for these files which isn't what we want for SSTableReaders on Windows: http://www.osronline.com/showThread.cfm?link=64732
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Sanity checked - with nio.2 on RAR and bypassing MappedByteBuffers in MmappedSegmentedFile, snapshot-based repairs on Windows work and clean up without issue. Not a solution long-term obviously but it helps support the hypothesis.

          I think it makes sense to 1) convert RAR to nio.2 and 2) add a SnapshotDeletingTask similar to the SSTableDeletingTask to delete snapshot files once the original sstables are compacted and unmapped. Alternatively we could allow snapshot files from repair to accrue and flag them for deletion on shutdown of the jvm and/or system reboot.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Sanity checked - with nio.2 on RAR and bypassing MappedByteBuffers in MmappedSegmentedFile, snapshot-based repairs on Windows work and clean up without issue. Not a solution long-term obviously but it helps support the hypothesis. I think it makes sense to 1) convert RAR to nio.2 and 2) add a SnapshotDeletingTask similar to the SSTableDeletingTask to delete snapshot files once the original sstables are compacted and unmapped. Alternatively we could allow snapshot files from repair to accrue and flag them for deletion on shutdown of the jvm and/or system reboot.
          Hide
          jbellis Jonathan Ellis added a comment - - edited

          I've said elsewhere that I'm fine with dropping the mapped i/o path in 3.0. It just isn't used often enough (and the performance difference isn't large enough) to justify the extra complexity.

          Show
          jbellis Jonathan Ellis added a comment - - edited I've said elsewhere that I'm fine with dropping the mapped i/o path in 3.0. It just isn't used often enough (and the performance difference isn't large enough) to justify the extra complexity.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited

          I'd like to drop the mapped i/o separately from converting the RAR over to nio.2 (assuming we want to go that route). You want me to open a new ticket for that and hammer that out as a pre-req to this?

          edit: also - do we have perf #'s from when we added the memory mapped i/o into the code-path?

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - - edited I'd like to drop the mapped i/o separately from converting the RAR over to nio.2 (assuming we want to go that route). You want me to open a new ticket for that and hammer that out as a pre-req to this? edit: also - do we have perf #'s from when we added the memory mapped i/o into the code-path?
          Hide
          jbellis Jonathan Ellis added a comment -

          I'd like to drop the mapped i/o separately

          WFM.

          do we have perf #'s from when we added the memory mapped i/o into the code-path

          It was a long time ago (CASSANDRA-408, CASSANDRA-669). It was a pretty big win at the time (10%? 20%?) because we were basically doing zero-copy reads from the mapped buffers. The problem is that we had to give that up when we started manually unmapping obsolete sstables (CASSANDRA-2521) – too hard to push refcounting all the way up the read path, so we gave up and just copy to a new buffer after all.

          Show
          jbellis Jonathan Ellis added a comment - I'd like to drop the mapped i/o separately WFM. do we have perf #'s from when we added the memory mapped i/o into the code-path It was a long time ago ( CASSANDRA-408 , CASSANDRA-669 ). It was a pretty big win at the time (10%? 20%?) because we were basically doing zero-copy reads from the mapped buffers. The problem is that we had to give that up when we started manually unmapping obsolete sstables ( CASSANDRA-2521 ) – too hard to push refcounting all the way up the read path, so we gave up and just copy to a new buffer after all.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Makes sense - it would require pushing some things pretty far back in the stack to hold a ref on a memory mapped segment on the read path.

          If we're thinking 3.X release for removing mmap I can throw a workaround on this ticket to always return a BufferedPoolingSegmentedFile from SegmentedFile's getBuilder if the platform is Windows. That + nio.2 should get us working snapshots and less weird file handle behaviors on Windows in 2.0.X without having to wait on clean-up of the old mmap code.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Makes sense - it would require pushing some things pretty far back in the stack to hold a ref on a memory mapped segment on the read path. If we're thinking 3.X release for removing mmap I can throw a workaround on this ticket to always return a BufferedPoolingSegmentedFile from SegmentedFile's getBuilder if the platform is Windows. That + nio.2 should get us working snapshots and less weird file handle behaviors on Windows in 2.0.X without having to wait on clean-up of the old mmap code.
          Hide
          jbellis Jonathan Ellis added a comment -

          Rewriting to nio2 is the same scope as removing mmap. Probably riskier actually. So 3.0 for both.

          Show
          jbellis Jonathan Ellis added a comment - Rewriting to nio2 is the same scope as removing mmap. Probably riskier actually. So 3.0 for both.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Fair enough. RAR is only the root of all our file i/o, after all. We should probably pursue either making snapshot deletion a quiet failure rather than deleteWithConfirm or disabling snapshot-based repair on Windows in 2.0.x. I'm inclined to go with the latter since I'd rather not disrupt the *nix ecosystem based on Windows file-system eccentricities in our current stabilization-phase.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Fair enough. RAR is only the root of all our file i/o, after all. We should probably pursue either making snapshot deletion a quiet failure rather than deleteWithConfirm or disabling snapshot-based repair on Windows in 2.0.x. I'm inclined to go with the latter since I'd rather not disrupt the *nix ecosystem based on Windows file-system eccentricities in our current stabilization-phase.
          Hide
          jbellis Jonathan Ellis added a comment -

          Created CASSANDRA-6907 for that.

          Show
          jbellis Jonathan Ellis added a comment - Created CASSANDRA-6907 for that.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Attaching 1st run at converting to nio.2. Test results on both Windows and linux at blends of 3/1, 50/1, 100/1 write/read ratios and the inverse look to be within margin of error, though we're not getting any huge gains out of this change. 3/1 sample:

          3/1 w/r test numbers
                   4050 mmap 3/1 r/w:
                                  id, ops       ,    op/s,adj op/s,   key/s,    mean,     med,     .95,     .99,    .999,     max,   time,   stderr
                       4 threadCount, 934400    ,   31029,   31030,   31029,     0.1,     0.1,     0.2,     0.2,     0.7,    48.3,   30.1,  0.01072
                       8 threadCount, 1259400   ,   41576,   41607,   41576,     0.2,     0.2,     0.2,     0.4,     1.1,    37.9,   30.3,  0.01139
                      16 threadCount, 1478350   ,   48565,   48592,   48565,     0.3,     0.3,     0.5,     1.0,     7.0,    73.6,   30.4,  0.01197
                      24 threadCount, 1523350   ,   49177,      -0,   49177,     0.5,     0.4,     0.7,     1.5,    19.1,    71.8,   31.0,  0.01668
                      36 threadCount, 1518900   ,   48679,   48718,   48679,     0.7,     0.6,     1.1,     2.3,    22.6,    92.7,   31.2,  0.01425
                      54 threadCount, 1541050   ,   48020,   48113,   48020,     1.1,     0.9,     1.8,     4.1,    28.6,   212.6,   32.1,  0.03217
                   trunk mmap 3/1 r/w:
                                  id, ops       ,    op/s,adj op/s,   key/s,    mean,     med,     .95,     .99,    .999,     max,   time,   stderr
                       4 threadCount, 926400    ,   30764,   30765,   30764,     0.1,     0.1,     0.2,     0.2,     0.7,    24.3,   30.1,  0.00997
                       8 threadCount, 1283250   ,   42495,      -0,   42495,     0.2,     0.2,     0.2,     0.3,     0.9,    44.4,   30.2,  0.01254
                      16 threadCount, 1478250   ,   48509,      -0,   48509,     0.3,     0.3,     0.5,     0.9,     4.1,    68.0,   30.5,  0.00912
                      24 threadCount, 1507900   ,   48553,   48594,   48553,     0.5,     0.4,     0.8,     1.7,    21.2,   132.1,   31.1,  0.01290
                      36 threadCount, 1515150   ,   48079,      -0,   48079,     0.7,     0.6,     1.2,     2.7,    23.3,   103.8,   31.5,  0.01531
                      54 threadCount, 1517600   ,   47826,      -0,   47826,     1.1,     0.9,     1.6,     3.2,    25.0,   194.4,   31.7,  0.01819
          

          I mention mmap in these results as using BufferedPoolingSegmentedFiles on both trunk and on this patch had a noticeable negative impact on throughput, more on nio.2 than on the byte[] raw usage. On trunk with read-heavy workloads I'm seeing anywhere from a 30-40% hit in stress results on read performance. 1/50 r/w ratio stress w/BufferedPoolingSegmentedFiles was still 16% slower than my testing using MmappedSegmentedFiles. I'll be attaching a sample of the perf #'s I've been getting to CASSANDRA-6890.

          I put some yammer timers inside the RAR code on both trunk and on this branch and it looks like #'s are comparable up to about the 60th percentile or so across all major read or rebuffer operations - then they balloon. In the territory of a max timestamp of 100+ms on a simple channel seek vs. .01 on mmap'ed. GC count during stress is roughly double at a glance - I'll look into that further on 6890 but heap stress due to more activity on the heap is to be expected.

          As noted earlier - in order to fully resolve this issue, either CASSANDRA-6890 will need to be resolved or some alternative solution for Windows if we keep mmap'ing in.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Attaching 1st run at converting to nio.2. Test results on both Windows and linux at blends of 3/1, 50/1, 100/1 write/read ratios and the inverse look to be within margin of error, though we're not getting any huge gains out of this change. 3/1 sample: 3/1 w/r test numbers 4050 mmap 3/1 r/w: id, ops , op/s,adj op/s, key/s, mean, med, .95, .99, .999, max, time, stderr 4 threadCount, 934400 , 31029, 31030, 31029, 0.1, 0.1, 0.2, 0.2, 0.7, 48.3, 30.1, 0.01072 8 threadCount, 1259400 , 41576, 41607, 41576, 0.2, 0.2, 0.2, 0.4, 1.1, 37.9, 30.3, 0.01139 16 threadCount, 1478350 , 48565, 48592, 48565, 0.3, 0.3, 0.5, 1.0, 7.0, 73.6, 30.4, 0.01197 24 threadCount, 1523350 , 49177, -0, 49177, 0.5, 0.4, 0.7, 1.5, 19.1, 71.8, 31.0, 0.01668 36 threadCount, 1518900 , 48679, 48718, 48679, 0.7, 0.6, 1.1, 2.3, 22.6, 92.7, 31.2, 0.01425 54 threadCount, 1541050 , 48020, 48113, 48020, 1.1, 0.9, 1.8, 4.1, 28.6, 212.6, 32.1, 0.03217 trunk mmap 3/1 r/w: id, ops , op/s,adj op/s, key/s, mean, med, .95, .99, .999, max, time, stderr 4 threadCount, 926400 , 30764, 30765, 30764, 0.1, 0.1, 0.2, 0.2, 0.7, 24.3, 30.1, 0.00997 8 threadCount, 1283250 , 42495, -0, 42495, 0.2, 0.2, 0.2, 0.3, 0.9, 44.4, 30.2, 0.01254 16 threadCount, 1478250 , 48509, -0, 48509, 0.3, 0.3, 0.5, 0.9, 4.1, 68.0, 30.5, 0.00912 24 threadCount, 1507900 , 48553, 48594, 48553, 0.5, 0.4, 0.8, 1.7, 21.2, 132.1, 31.1, 0.01290 36 threadCount, 1515150 , 48079, -0, 48079, 0.7, 0.6, 1.2, 2.7, 23.3, 103.8, 31.5, 0.01531 54 threadCount, 1517600 , 47826, -0, 47826, 1.1, 0.9, 1.6, 3.2, 25.0, 194.4, 31.7, 0.01819 I mention mmap in these results as using BufferedPoolingSegmentedFiles on both trunk and on this patch had a noticeable negative impact on throughput, more on nio.2 than on the byte[] raw usage. On trunk with read-heavy workloads I'm seeing anywhere from a 30-40% hit in stress results on read performance. 1/50 r/w ratio stress w/BufferedPoolingSegmentedFiles was still 16% slower than my testing using MmappedSegmentedFiles. I'll be attaching a sample of the perf #'s I've been getting to CASSANDRA-6890 . I put some yammer timers inside the RAR code on both trunk and on this branch and it looks like #'s are comparable up to about the 60th percentile or so across all major read or rebuffer operations - then they balloon. In the territory of a max timestamp of 100+ms on a simple channel seek vs. .01 on mmap'ed. GC count during stress is roughly double at a glance - I'll look into that further on 6890 but heap stress due to more activity on the heap is to be expected. As noted earlier - in order to fully resolve this issue, either CASSANDRA-6890 will need to be resolved or some alternative solution for Windows if we keep mmap'ing in.
          Hide
          jbellis Jonathan Ellis added a comment -

          Benedict to review

          Show
          jbellis Jonathan Ellis added a comment - Benedict to review
          Hide
          benedict Benedict added a comment -

          I've uploaded a tidied up version here

          I've eliminated some unnecessary variables, simplified a couple of loops/conditions, and unified the AbstractDataInput/Small hierarchy. Also fixed a minor "bug" with getPosition() in RAR after close(), and CRAR now ensures that the current position is restored after rebuffer() - whilst currently this wouldn't cause any problems, it seems like an oversight.

          Show
          benedict Benedict added a comment - I've uploaded a tidied up version here I've eliminated some unnecessary variables, simplified a couple of loops/conditions, and unified the AbstractDataInput/Small hierarchy. Also fixed a minor "bug" with getPosition() in RAR after close(), and CRAR now ensures that the current position is restored after rebuffer() - whilst currently this wouldn't cause any problems, it seems like an oversight.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Good catch on getPosition - I accounted for that in current() but that hadn't triggered on any testing and was an oversight.

          I kept AbstractDataInput and AbstractDataInputSmall separate in the type heirarchy because I didn't want to push the int -> long signature change down to all the classes that implemented the base. I'm not sure if the added footprint justifies the added complexity or not - I was trying to minimize changes to unrelated classes due to the loss of RAF code. I didn't like it, but I also don'e like the alternative that much. It looks like we run the risk of Bad Things if someone does a MemoryInputStream.skipBytes that pushes the position past Max Int - this impl has us casting off the remainder on a seek call so you could end up in negative territory.

          As for the tidying up - looks good to me. Thanks for taking the time to do that - clean idiomatic usage of the nio API's clearly makes things easier to parse.

          Tests on linux look good, snapshots on Windows behave w/benedict's revisions and no mmap, and read performance looks comparable so I +1 the changes with the above caveat.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Good catch on getPosition - I accounted for that in current() but that hadn't triggered on any testing and was an oversight. I kept AbstractDataInput and AbstractDataInputSmall separate in the type heirarchy because I didn't want to push the int -> long signature change down to all the classes that implemented the base. I'm not sure if the added footprint justifies the added complexity or not - I was trying to minimize changes to unrelated classes due to the loss of RAF code. I didn't like it, but I also don'e like the alternative that much. It looks like we run the risk of Bad Things if someone does a MemoryInputStream.skipBytes that pushes the position past Max Int - this impl has us casting off the remainder on a seek call so you could end up in negative territory. As for the tidying up - looks good to me. Thanks for taking the time to do that - clean idiomatic usage of the nio API's clearly makes things easier to parse. Tests on linux look good, snapshots on Windows behave w/benedict's revisions and no mmap, and read performance looks comparable so I +1 the changes with the above caveat.
          Hide
          benedict Benedict added a comment -

          It looks like we run the risk of Bad Things if someone does a MemoryInputStream.skipBytes that pushes the position past Max Int - this impl has us casting off the remainder on a seek call so you could end up in negative territory.

          How so? The MemoryInputStream defines what its limit is, and the skipBytes method ensures it never goes above this. So seek() can never be called with a value that is out of range (since it is a protected method). We could put in an assert if we want to be doubly certain, however, and that's probably not a bad idea for simple declaration of intent.

          I think the reduced code duplication (from readLine and skipBytes now being shared), and cleaner hierarchy is preferable, especially as ADISmall is not a very clear distinction from ADI. Think the overall footprint is reduced rather than increased...?

          Thanks for taking the time to do that - clean idiomatic usage of the nio API's clearly makes things easier to parse.

          I find the NIO library tough to parse at the best of times, and wanted to be sure I was reading it right, so it was a freebie to change as I reviewed

          Show
          benedict Benedict added a comment - It looks like we run the risk of Bad Things if someone does a MemoryInputStream.skipBytes that pushes the position past Max Int - this impl has us casting off the remainder on a seek call so you could end up in negative territory. How so? The MemoryInputStream defines what its limit is, and the skipBytes method ensures it never goes above this. So seek() can never be called with a value that is out of range (since it is a protected method). We could put in an assert if we want to be doubly certain, however, and that's probably not a bad idea for simple declaration of intent. I think the reduced code duplication (from readLine and skipBytes now being shared), and cleaner hierarchy is preferable, especially as ADISmall is not a very clear distinction from ADI. Think the overall footprint is reduced rather than increased...? Thanks for taking the time to do that - clean idiomatic usage of the nio API's clearly makes things easier to parse. I find the NIO library tough to parse at the best of times, and wanted to be sure I was reading it right, so it was a freebie to change as I reviewed
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          the skipBytes method ensures it never goes above this

          How is skipBytes protecting against blowing past our limit? (note: me just being dense here is not out of the question)

           64     public int skipBytes(int n) throws IOException
           65     {
           66         if (n <= 0)
           67             return 0;
           68         seek(getPosition() + n);
           69         return position;
           70     }
          

          It looks like this exposes seek() to the outside world with a protection against negative inputs but not much else. That being said - the old code looks like it has the same potential problem:

              public int skipBytes(int n) throws IOException
              {
                  seekInternal(getPosition() + n);
                  return position;
              }
          
          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - the skipBytes method ensures it never goes above this How is skipBytes protecting against blowing past our limit? (note: me just being dense here is not out of the question) 64 public int skipBytes( int n) throws IOException 65 { 66 if (n <= 0) 67 return 0; 68 seek(getPosition() + n); 69 return position; 70 } It looks like this exposes seek() to the outside world with a protection against negative inputs but not much else. That being said - the old code looks like it has the same potential problem: public int skipBytes( int n) throws IOException { seekInternal(getPosition() + n); return position; }
          Hide
          benedict Benedict added a comment -

          Ah, this is my failure to delete the skipBytes method from MIS, as it now occurs in ADI (in a safe manner).

          Show
          benedict Benedict added a comment - Ah, this is my failure to delete the skipBytes method from MIS, as it now occurs in ADI (in a safe manner).
          Hide
          benedict Benedict added a comment -

          In fact, it looks like that is simply a bug that has always been present - the new behaviour is no worse than the old, but deleting it is still the correct fix.

          Good spot.

          Show
          benedict Benedict added a comment - In fact, it looks like that is simply a bug that has always been present - the new behaviour is no worse than the old, but deleting it is still the correct fix. Good spot.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Sure enough. given the docs for skipBytes are 0-n bytes skipped I think the code in ADI looks good. I'd much rather we not add more types to the hierarchy in this context.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Sure enough. given the docs for skipBytes are 0-n bytes skipped I think the code in ADI looks good. I'd much rather we not add more types to the hierarchy in this context.
          Hide
          benedict Benedict added a comment -

          Sounds like we're in agreement then? Any further changes you want to make after my update?

          Show
          benedict Benedict added a comment - Sounds like we're in agreement then? Any further changes you want to make after my update?
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          No other changes. Depending on where we fall on CASSANDRA-6890 I may want to expand this ticket to cover using SSTableDeletingTasks on snapshot files to work around deleting memory mapped file segments on Windows in order to fix the issue that caused us to go down this path in the first place. I don't like low-level I/O rewrites being under the guise of a Windows snapshot file ticket so I may do some housekeeping here.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - No other changes. Depending on where we fall on CASSANDRA-6890 I may want to expand this ticket to cover using SSTableDeletingTasks on snapshot files to work around deleting memory mapped file segments on Windows in order to fix the issue that caused us to go down this path in the first place. I don't like low-level I/O rewrites being under the guise of a Windows snapshot file ticket so I may do some housekeeping here.
          Hide
          benedict Benedict added a comment -

          Rename/describe the ticket?

          I had been thinking the same thing.

          Show
          benedict Benedict added a comment - Rename/describe the ticket? I had been thinking the same thing.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          I'll rebase your branch against trunk and post a revised patch early next week. I know how much you love rebasing and I figure I owe you one for the house-cleaning on this patch.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - I'll rebase your branch against trunk and post a revised patch early next week. I know how much you love rebasing and I figure I owe you one for the house-cleaning on this patch.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          New patch attached. Passes the same tests trunk does and perf is in line.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - New patch attached. Passes the same tests trunk does and perf is in line.
          Hide
          benedict Benedict added a comment -

          Might want to delete skipBytes from MIS as we discussed, otherwise LGTM and ready for commit.

          Show
          benedict Benedict added a comment - Might want to delete skipBytes from MIS as we discussed, otherwise LGTM and ready for commit.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Odd. I wonder if I didn't pull down that commit before I rebased locally. Odd since I thought I saw that on the difftool run; I'll fix it and repost.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Odd. I wonder if I didn't pull down that commit before I rebased locally. Odd since I thought I saw that on the difftool run; I'll fix it and repost.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          v3 attached. I removed the CommitLogTest changes that snuck on on the remove skipBytes from MIS commit from you as they looked unrelated to this ticket.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - v3 attached. I removed the CommitLogTest changes that snuck on on the remove skipBytes from MIS commit from you as they looked unrelated to this ticket.
          Hide
          benedict Benedict added a comment -

          +1

          Show
          benedict Benedict added a comment - +1
          Hide
          jbellis Jonathan Ellis added a comment -

          committed

          Show
          jbellis Jonathan Ellis added a comment - committed

            People

            • Assignee:
              JoshuaMcKenzie Joshua McKenzie
              Reporter:
              jn Jim Newsham
              Reviewer:
              Benedict
            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development