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

ZkConfigManager filesystem separator fix for Windows

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.1
    • Component/s: None
    • Labels:
      None

      Description

      The separator for zk nodes is '/'. However, on Windows, while uploading the relative files nested within a directory (e.g. velocity\hit-plain.vm) contain '\'. This, apart from causing an inconsistency as compared to zk on posix systems, messed up the ZkCLITest on Windows where count of files in zk and files in the filesystem was compared.

      1. SOLR-7158.patch
        2 kB
        Alan Woodward
      2. SOLR-7158.patch
        2 kB
        Ishan Chattopadhyaya

        Issue Links

          Activity

          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Attaching a patch to fix this; should fix the jenkins failure for the ZkCLITest.testUpConfigLinkConfigClearZk().

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Attaching a patch to fix this; should fix the jenkins failure for the ZkCLITest.testUpConfigLinkConfigClearZk().
          Hide
          romseygeek Alan Woodward added a comment -

          Thanks Ishan!

          Is the second Files.createDirectories() actually necessary? It's already called at the top of downloadFromZk

          Show
          romseygeek Alan Woodward added a comment - Thanks Ishan! Is the second Files.createDirectories() actually necessary? It's already called at the top of downloadFromZk
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Thanks for reviewing, Alan.
          The first createDirectories() (on the top) creates the base directory, e.g. c:\users\ishan\configs.
          The one I added creates the directories for nested files in the ZK configs, e.g. for a file in zk: velocity\plain-hit.vm it creates c:\users\ishan\configs\velocity.

          I think the first one (on top) could be removed, since the latter one will recursively create the base directory as well (something like mkdir -p).

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Thanks for reviewing, Alan. The first createDirectories() (on the top) creates the base directory, e.g. c:\users\ishan\configs. The one I added creates the directories for nested files in the ZK configs, e.g. for a file in zk: velocity\plain-hit.vm it creates c:\users\ishan\configs\velocity. I think the first one (on top) could be removed, since the latter one will recursively create the base directory as well (something like mkdir -p).
          Hide
          romseygeek Alan Woodward added a comment -

          Right, but downloadFromZk is called recursively for every subdirectory, so the createDirectories() at the top should be called for each directory that needs to be constructed. Unless I'm missing something?

          Show
          romseygeek Alan Woodward added a comment - Right, but downloadFromZk is called recursively for every subdirectory, so the createDirectories() at the top should be called for each directory that needs to be constructed. Unless I'm missing something?
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Ah, I see what you mean! Actually, because of the second issue with the path separator, something like velocity\plain-hit.vm was not being considered as a subpath, and thus the Files.write() was attempting to create a single file with that name (velocity\plain-hit.vm), which failed. Hence, I tried to explicitly create the sub directory (velocity) by calling createDirectories(filename.getParent()), which in view of the path separator fix seems redundant. Thanks for the catch!

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Ah, I see what you mean! Actually, because of the second issue with the path separator, something like velocity\plain-hit.vm was not being considered as a subpath, and thus the Files.write() was attempting to create a single file with that name (velocity\plain-hit.vm), which failed. Hence, I tried to explicitly create the sub directory (velocity) by calling createDirectories(filename.getParent()), which in view of the path separator fix seems redundant. Thanks for the catch!
          Hide
          romseygeek Alan Woodward added a comment -

          Here's an alternative patch (with Windows path shenanigans extracted to a helper method). Could you test this on a Windows box? Thanks!

          Show
          romseygeek Alan Woodward added a comment - Here's an alternative patch (with Windows path shenanigans extracted to a helper method). Could you test this on a Windows box? Thanks!
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          I like the path separator logic being exctracted into a helper method. Just checked on Windows, it works fine!
          A quick lookup at http://en.wikipedia.org/wiki/Path_%28computing%29#Representations_of_paths_by_operating_system_and_shell convinces me that we can actually hardcode "\" for the check for separator (as you've done here). That might also make it fine (more performant?) to replace Pattern.quote(separator) with "\\\\"?

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited I like the path separator logic being exctracted into a helper method. Just checked on Windows, it works fine! A quick lookup at http://en.wikipedia.org/wiki/Path_%28computing%29#Representations_of_paths_by_operating_system_and_shell convinces me that we can actually hardcode "\" for the check for separator (as you've done here). That might also make it fine (more performant?) to replace Pattern.quote(separator) with "\\\\"?
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Just a thought, instead of createZkNode (which gives the impression that it actually creates a zk node), can we rename the helper method to getZkNodeName/createZkNodeName or something similar?

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Just a thought, instead of createZkNode (which gives the impression that it actually creates a zk node), can we rename the helper method to getZkNodeName/createZkNodeName or something similar?
          Hide
          romseygeek Alan Woodward added a comment -

          I've incorporated both suggestions. Thanks again!

          Show
          romseygeek Alan Woodward added a comment - I've incorporated both suggestions. Thanks again!
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1662205 from Alan Woodward in branch 'dev/trunk'
          [ https://svn.apache.org/r1662205 ]

          SOLR-7158: Fix zk upload on Windows systems

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1662205 from Alan Woodward in branch 'dev/trunk' [ https://svn.apache.org/r1662205 ] SOLR-7158 : Fix zk upload on Windows systems
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 1662206 from Alan Woodward in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1662206 ]

          SOLR-7158: Fix zk upload on Windows systems

          Show
          jira-bot ASF subversion and git services added a comment - Commit 1662206 from Alan Woodward in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1662206 ] SOLR-7158 : Fix zk upload on Windows systems
          Hide
          thelabdude Timothy Potter added a comment -

          Bulk close after 5.1 release

          Show
          thelabdude Timothy Potter added a comment - Bulk close after 5.1 release

            People

            • Assignee:
              romseygeek Alan Woodward
              Reporter:
              ichattopadhyaya Ishan Chattopadhyaya
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development