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.

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.

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.

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.

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

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.

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

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.

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.

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?

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.

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.

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

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.

Created CASSANDRA-6907 for that.

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:

         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.

Benedict to review

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.

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.

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

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;
    }

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

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.

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.

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

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.

Rename/describe the ticket?

I had been thinking the same thing.

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.

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

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

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.

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.

+1

committed