# ZkConfigManager filesystem separator fix for Windows

## Details

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: None
• Fix Version/s:
• 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.

## Attachments

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

## Activity

Hide

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!

Show
Hide

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

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

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

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: