Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-10387

zkTransfer normalizes destination path incorrectly if source is a windows directory

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.6, 7.0
    • Component/s: None
    • Security Level: Public (Default Security Level. Issues are Public)
    • Labels:
      None

      Description

      While normalizing dest it is looking only for '/' in source path but this will not work for windows style delimiter.

      /lucene-solr/solr/solrj/src/java/org/apache/solr/common/cloud/ZkMaintenanceUtils.java

      private static String normalizeDest(String srcName, String dstName) {
      if (dstName.endsWith("/")) { // Dest is a directory.
      int pos = srcName.lastIndexOf("/");
      if (pos < 0) {

      1. SOLR-10387.patch
        8 kB
        Erick Erickson
      2. SOLR-10387.patch
        7 kB
        Erick Erickson
      3. SOLR-10387.patch
        5 kB
        Erick Erickson
      4. SOLR-10387.patch
        5 kB
        gopikannan venugopalsamy
      5. SOLR-10387.patch
        1 kB
        Erick Erickson

        Issue Links

          Activity

          Hide
          erickerickson Erick Erickson added a comment -

          Ah, thanks. This may well be the recent test failures that I was about to try to track down. Figured it would be something to do with file separators. I just did a visual check and I think I've corrected the problems. All of the remaining references to forward slash are ZooKeeper paths which don't have this problem.

          I don't have access to Windows machines to test, so if you could do any or all of these it would be an enormous favor:

          1> try running

          ant test -Dtestcase=SolrCLIZkUtilsTest -Dtests.method=testCp -Dtests.seed=65C25EBE642D0428 -Dtests.slow=true -Dtests.locale=zh-CN -Dtests.timezone=Indian/Comoro -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1

          That's the failing Jenkins test. I want it to fail for you too before the patch.

          2> Apply the patch I'll attach to this JIRA. It's against trunk but it should apply to 6x.

          3> run the test in <1> again, I want it to succeed....

          4> run through the scenario I outlined in SOLR-10108 on 19-Feb (here are the steps, there's some more explanation on that JIRA):
          > Create a collection on Solr, any collection will do.
          > 'bin/solr zk cp -r zk:/ file:/some/dir -z localhost:2181'
          > Shut down Solr
          > Re-initialize ZK (wipe all the data)
          > 'bin/solr zk cp -r file:/some/dir/ zk:/ -z localhost:2181'
          > start Solr again
          > You should see everything just as it was before.

          5> take a quick look at the help screens for cp ('bin/solr.cmd zk -help' should do it).

          Please feel free to attach a patch or just a corrected solr.cmd would be great too.

          Thanks!

          Show
          erickerickson Erick Erickson added a comment - Ah, thanks. This may well be the recent test failures that I was about to try to track down. Figured it would be something to do with file separators. I just did a visual check and I think I've corrected the problems. All of the remaining references to forward slash are ZooKeeper paths which don't have this problem. I don't have access to Windows machines to test, so if you could do any or all of these it would be an enormous favor: 1> try running ant test -Dtestcase=SolrCLIZkUtilsTest -Dtests.method=testCp -Dtests.seed=65C25EBE642D0428 -Dtests.slow=true -Dtests.locale=zh-CN -Dtests.timezone=Indian/Comoro -Dtests.asserts=true -Dtests.file.encoding=ISO-8859-1 That's the failing Jenkins test. I want it to fail for you too before the patch. 2> Apply the patch I'll attach to this JIRA. It's against trunk but it should apply to 6x. 3> run the test in <1> again, I want it to succeed.... 4> run through the scenario I outlined in SOLR-10108 on 19-Feb (here are the steps, there's some more explanation on that JIRA): > Create a collection on Solr, any collection will do. > 'bin/solr zk cp -r zk:/ file:/some/dir -z localhost:2181' > Shut down Solr > Re-initialize ZK (wipe all the data) > 'bin/solr zk cp -r file:/some/dir/ zk:/ -z localhost:2181' > start Solr again > You should see everything just as it was before. 5> take a quick look at the help screens for cp ('bin/solr.cmd zk -help' should do it). Please feel free to attach a patch or just a corrected solr.cmd would be great too. Thanks!
          Hide
          erickerickson Erick Erickson added a comment - - edited

          Uploaded too soon, give me a few. Never mind, must have been hallucinating. Go ahead and try if if you can.

          Show
          erickerickson Erick Erickson added a comment - - edited Uploaded too soon, give me a few. Never mind, must have been hallucinating. Go ahead and try if if you can.
          Hide
          gopikannan gopikannan venugopalsamy added a comment -

          No, That did not work. The comment and code is not matching, As per comment we have to make checks on src not dest. Which one is right?
          // Pull the last element of the src path and add it to the dst if the src does NOT end in a slash
          // If the source ends in a slash, do not append the last segment to the dest
          if (dstName.endsWith("/")) { // Dest is a directory.

          Show
          gopikannan gopikannan venugopalsamy added a comment - No, That did not work. The comment and code is not matching, As per comment we have to make checks on src not dest. Which one is right? // Pull the last element of the src path and add it to the dst if the src does NOT end in a slash // If the source ends in a slash, do not append the last segment to the dest if (dstName.endsWith("/")) { // Dest is a directory.
          Hide
          gopikannan gopikannan venugopalsamy added a comment - - edited

          Please check the patch I have attached, Tests succeed with it.

          If dest ends with separator or '/' depending on whether it is zk or local path, last component of source (again separator of source is '/' if is zk else local delimiter) is appended to dest.

          Show
          gopikannan gopikannan venugopalsamy added a comment - - edited Please check the patch I have attached, Tests succeed with it. If dest ends with separator or '/' depending on whether it is zk or local path, last component of source (again separator of source is '/' if is zk else local delimiter) is appended to dest.
          Hide
          erickerickson Erick Erickson added a comment -

          I did change the comment as you're right it's confused. I dressed it up and put it above the method.

          That's not the code in the patch though. The first patch reads:

          // If the dest ends in a file separator, it's a directory so append the last element of the src to it.
          if (dstName.endsWith(File.separator)) { // Dest is a directory.
          int pos = srcName.lastIndexOf(File.separator);

          Actually, looking at it some more the patch is wrong anyway, I'll upload another version. The old one worked for me since the local fs separator is '/'. The above now reads as below:

          String dstSeparator = (dstIsZk) ? "/" : File.separator;
          String srcSeparator = (srcIsZk) ? "/" : File.separator;

          if (dstName.endsWith(dstSeparator)) { // Dest is a directory or non-leaf znode.
          int pos = srcName.lastIndexOf(srcSeparator);

          If this doesn't work for you, can you tell me a little more about how it fails?

          Sorry for the run-around...

          Show
          erickerickson Erick Erickson added a comment - I did change the comment as you're right it's confused. I dressed it up and put it above the method. That's not the code in the patch though. The first patch reads: // If the dest ends in a file separator, it's a directory so append the last element of the src to it. if (dstName.endsWith(File.separator)) { // Dest is a directory. int pos = srcName.lastIndexOf(File.separator); Actually, looking at it some more the patch is wrong anyway, I'll upload another version. The old one worked for me since the local fs separator is '/'. The above now reads as below: String dstSeparator = (dstIsZk) ? "/" : File.separator; String srcSeparator = (srcIsZk) ? "/" : File.separator; if (dstName.endsWith(dstSeparator)) { // Dest is a directory or non-leaf znode. int pos = srcName.lastIndexOf(srcSeparator); If this doesn't work for you, can you tell me a little more about how it fails? Sorry for the run-around...
          Hide
          gopikannan gopikannan venugopalsamy added a comment -

          Did you check the second patch I attached ? Tests passed with it.

          Show
          gopikannan gopikannan venugopalsamy added a comment - Did you check the second patch I attached ? Tests passed with it.
          Hide
          erickerickson Erick Erickson added a comment -

          Too funny! We both did he exact same solution in the normalizeDest method, clear to naming the variables identically and passing in srcIsZk and dstIsZk as booleans.

          I was looking at the code in detail though, and I believe your patch doesn't handle the case of copying a single file from ZK to a local directory. This is the last section in the method, the patch I just put up has this comment just above the code in question:

          // Single file ZK -> local copy where ZK is a leaf node
          The test there is still
          if (dst.endsWith("/") == false) dst += "/";

          give me a minute to incorporate your test changes though.

          Show
          erickerickson Erick Erickson added a comment - Too funny! We both did he exact same solution in the normalizeDest method, clear to naming the variables identically and passing in srcIsZk and dstIsZk as booleans. I was looking at the code in detail though, and I believe your patch doesn't handle the case of copying a single file from ZK to a local directory. This is the last section in the method, the patch I just put up has this comment just above the code in question: // Single file ZK -> local copy where ZK is a leaf node The test there is still if (dst.endsWith("/") == false) dst += "/"; give me a minute to incorporate your test changes though.
          Hide
          erickerickson Erick Erickson added a comment -

          I didn't notice you'd put up a patch until after I had put up mine.

          See the comment though about copying single files from ZK->local. Do you agree?

          This patch incorporates your test changes and our code fix.

          Show
          erickerickson Erick Erickson added a comment - I didn't notice you'd put up a patch until after I had put up mine. See the comment though about copying single files from ZK->local. Do you agree? This patch incorporates your test changes and our code fix.
          Hide
          gopikannan gopikannan venugopalsamy added a comment -

          Agreed, Tried the last patch, Tests went through. Thanks.

          Show
          gopikannan gopikannan venugopalsamy added a comment - Agreed, Tried the last patch, Tests went through. Thanks.
          Hide
          erickerickson Erick Erickson added a comment -

          And thank you. I've been feeling guilty that I haven't been able to test this on Windows. I'll commit this this evening.

          Show
          erickerickson Erick Erickson added a comment - And thank you . I've been feeling guilty that I haven't been able to test this on Windows. I'll commit this this evening.
          Hide
          erickerickson Erick Erickson added a comment -

          Final patch with CHANGES.txt

          Show
          erickerickson Erick Erickson added a comment - Final patch with CHANGES.txt
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit edcdc3052ba95840593ace32d6d9a7a6e4ebe7ea in lucene-solr's branch refs/heads/master from Erick Erickson
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=edcdc30 ]

          SOLR-10387: zkTransfer normalizes destination path incorrectly if source is a windows directory

          Show
          jira-bot ASF subversion and git services added a comment - Commit edcdc3052ba95840593ace32d6d9a7a6e4ebe7ea in lucene-solr's branch refs/heads/master from Erick Erickson [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=edcdc30 ] SOLR-10387 : zkTransfer normalizes destination path incorrectly if source is a windows directory
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 65982895f9d8c7437d912c12008ae4d9c4d87220 in lucene-solr's branch refs/heads/branch_6x from Erick Erickson
          [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6598289 ]

          SOLR-10387: zkTransfer normalizes destination path incorrectly if source is a windows directory

          (cherry picked from commit edcdc30)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 65982895f9d8c7437d912c12008ae4d9c4d87220 in lucene-solr's branch refs/heads/branch_6x from Erick Erickson [ https://git-wip-us.apache.org/repos/asf?p=lucene-solr.git;h=6598289 ] SOLR-10387 : zkTransfer normalizes destination path incorrectly if source is a windows directory (cherry picked from commit edcdc30)
          Hide
          erickerickson Erick Erickson added a comment -

          Thanks gopikannan!

          Show
          erickerickson Erick Erickson added a comment - Thanks gopikannan!

            People

            • Assignee:
              erickerickson Erick Erickson
              Reporter:
              gopikannan gopikannan venugopalsamy
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development