Solr
  1. Solr
  2. SOLR-7158

ZkConfigManager filesystem separator fix for Windows

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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
          Ishan Chattopadhyaya added a comment -

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

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

          Thanks Ishan!

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

          Show
          Alan Woodward added a comment - Thanks Ishan! Is the second Files.createDirectories() actually necessary? It's already called at the top of downloadFromZk
          Hide
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          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
          Alan Woodward added a comment -

          I've incorporated both suggestions. Thanks again!

          Show
          Alan Woodward added a comment - I've incorporated both suggestions. Thanks again!
          Hide
          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
          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
          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
          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
          Timothy Potter added a comment -

          Bulk close after 5.1 release

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

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development