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

Windows: remove mmap'ed I/O for index files and force standard file access

    Details

      Description

      Memory-mapped I/O on Windows causes issues with hard-links; we're unable to delete hard-links to open files with memory-mapped segments even using nio. We'll need to push for close to performance parity between mmap'ed I/O and buffered going forward as the buffered / compressed path offers other benefits.

      1. 6993_2.1_v1.txt
        4 kB
        Joshua McKenzie
      2. 6993_v1.txt
        4 kB
        Joshua McKenzie
      3. 6993_v2.txt
        4 kB
        Joshua McKenzie
      4. 6993_v3.txt
        3 kB
        Joshua McKenzie

        Issue Links

          Activity

          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Hard-coded in DatabaseDescriptor to use Config.DiskAccessMode.standard for both file and index access if it's not a unix environment.

          I also took the liberty of changing the isUnix check to strcmp once on class init and then reference a set boolean later rather than checking the string every time we called the function.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Hard-coded in DatabaseDescriptor to use Config.DiskAccessMode.standard for both file and index access if it's not a unix environment. I also took the liberty of changing the isUnix check to strcmp once on class init and then reference a set boolean later rather than checking the string every time we called the function.
          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 -

          1) isUnix should be "final"
          2) I think your "isUnix" check is too limited: this will break Mac OSX, FreeBSD and Solaris users, possibly others. Since basically every OS other than Windows probably supports this, I'd suggest making it an "isWindows" check and looking for contains("windows"). This link may help, although may not be completely authoritative. A quick grep of openjdk shows the following line in their own test tools, though:

          static boolean isWindows = System.getProperty("os.name").startsWith("Windows");
          

          Which suggests it's probably sufficient.

          Show
          benedict Benedict added a comment - 1) isUnix should be "final" 2) I think your "isUnix" check is too limited: this will break Mac OSX, FreeBSD and Solaris users, possibly others. Since basically every OS other than Windows probably supports this, I'd suggest making it an "isWindows" check and looking for contains("windows"). This link may help, although may not be completely authoritative. A quick grep of openjdk shows the following line in their own test tools, though: static boolean isWindows = System .getProperty( "os.name" ).startsWith( "Windows" ); Which suggests it's probably sufficient.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Good call on the final.

          I also agree on the logic - I simply re-factored the existing logic and took its correctness at face-value since it was already committed into the code-base.

          I'll clean these two things up and post a revised patch - thanks for the feedback.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Good call on the final. I also agree on the logic - I simply re-factored the existing logic and took its correctness at face-value since it was already committed into the code-base. I'll clean these two things up and post a revised patch - thanks for the feedback.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Attached v2. FBUtilities.isUnix() is really checking whether we're windows or not, but for the purposes of the things we check for it's accurate. Confirmed on Win8 and Win7 that this check behaves itself (Windows 8 and Windows 7 are output respectively).

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Attached v2. FBUtilities.isUnix() is really checking whether we're windows or not, but for the purposes of the things we check for it's accurate. Confirmed on Win8 and Win7 that this check behaves itself (Windows 8 and Windows 7 are output respectively).
          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
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          We should probably backport this for the 2.1.3 release as memory mapped I/O is going to cause the same problems there and people are beta-testing the software on Windows.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - We should probably backport this for the 2.1.3 release as memory mapped I/O is going to cause the same problems there and people are beta-testing the software on Windows.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          rebased patch attached

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - rebased patch attached
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          ping Benedict. Looking for a sanity check that there's no problem disabling mmap'ed index files on the 2.1 branch on Windows that I'm not thinking of rather than a code review.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - ping Benedict . Looking for a sanity check that there's no problem disabling mmap'ed index files on the 2.1 branch on Windows that I'm not thinking of rather than a code review.
          Hide
          benedict Benedict added a comment -

          Replacing isUnix() with !isWindows() is not functionally equivalent; this will capture Mac, Solaris, OpenBSD, FreeBSD and others as well, although in many situations this actually adequately captures what we want (such as for your specific change) it likely won't in all cases.

          As with CASSANRA-8038 this would benefit from sanitising our OS detection. Perhaps we could split this out into a minor ticket these both depend upon, as we have a bit of a mess right now that permits these sorts of logical mismatches to crop up. We should probably group POSIX compliant OSes together, and POSIX compliant file systems together, one of which is probably what we generally mean when we say isUnix().

          Show
          benedict Benedict added a comment - Replacing isUnix() with !isWindows() is not functionally equivalent; this will capture Mac, Solaris, OpenBSD, FreeBSD and others as well, although in many situations this actually adequately captures what we want (such as for your specific change) it likely won't in all cases. As with CASSANRA-8038 this would benefit from sanitising our OS detection. Perhaps we could split this out into a minor ticket these both depend upon, as we have a bit of a mess right now that permits these sorts of logical mismatches to crop up. We should probably group POSIX compliant OSes together, and POSIX compliant file systems together, one of which is probably what we generally mean when we say isUnix().
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Very good point. Looking through the code-base, every place where we're using isUnix seems to really mean 'isn't Windows' so I'd be comfortable with that distinction for now with the ability to make it more complex/powerful in the future if necessary. Also, right now we're skipping early re-open on files based on that check for FBUtilities.isUnix (see CASSANDRA-7365) so I'd prefer to get this modification in before 2.1.3 so we can get more coverage / usage of the early re-open logic, at least on OSX-based dev machines.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Very good point. Looking through the code-base, every place where we're using isUnix seems to really mean 'isn't Windows' so I'd be comfortable with that distinction for now with the ability to make it more complex/powerful in the future if necessary. Also, right now we're skipping early re-open on files based on that check for FBUtilities.isUnix (see CASSANDRA-7365 ) so I'd prefer to get this modification in before 2.1.3 so we can get more coverage / usage of the early re-open logic, at least on OSX-based dev machines.
          Hide
          benedict Benedict added a comment -

          This wouldn't be sufficient for the procfs check, as Mac (and by default FreeBSD) don't have it.

          Show
          benedict Benedict added a comment - This wouldn't be sufficient for the procfs check, as Mac (and by default FreeBSD) don't have it.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          Waiting for CASSANDRA-8452 to shake out, then I'll rebase this to that spec.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - Waiting for CASSANDRA-8452 to shake out, then I'll rebase this to that spec.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          v3 attached now that the windows/proc checks are sorted out.

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - v3 attached now that the windows/proc checks are sorted out.
          Hide
          benedict Benedict added a comment -

          Might as well normalise the log message from "Non-unix" to "Windows". Otherwise +1.

          Show
          benedict Benedict added a comment - Might as well normalise the log message from "Non-unix" to "Windows". Otherwise +1.
          Hide
          JoshuaMcKenzie Joshua McKenzie added a comment -

          committed

          Show
          JoshuaMcKenzie Joshua McKenzie added a comment - committed

            People

            • Assignee:
              JoshuaMcKenzie Joshua McKenzie
              Reporter:
              JoshuaMcKenzie Joshua McKenzie
              Reviewer:
              Benedict
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development