ZOOKEEPER-5037: Add export and import commands to the CLI

[ckrumbein]

This PR adds two commands to the ZooKeeper CLI, export and import:

• "export <path> <filepath>" downloads the contents of a znode to an external file.
• "import <path> <filepath>" uploads the contents of an external file to a znode, replacing the znode's previous contents.

My team has been using both of these commands for some time. We are confident they are solid.

[PDavid]

Many thanks for this contribution. These commands work really nicely. 👍

Added some minor comments / ideas.

[PDavid]

Checkstyle identified some formatting problems in the build:

02:52:13  [INFO] There are 8 errors reported by Checkstyle 8.39 with checkstyle-strict.xml ruleset.
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ExportCommand.java:[28,1] (imports) ImportOrder: Extra separation in import group before 'java.io.IOException'
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ExportCommand.java:[28,1] (imports) ImportOrder: Import java.io.IOException appears after other imports that it should precede
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ExportCommand.java:[79,33] (whitespace) WhitespaceAround: '|' is not followed by whitespace.
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ExportCommand.java:[79,33] (whitespace) WhitespaceAround: '|' is not preceded with whitespace.
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ImportCommand.java:[28,1] (imports) ImportOrder: Extra separation in import group before 'java.io.IOException'
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ImportCommand.java:[28,1] (imports) ImportOrder: Import java.io.IOException appears after other imports that it should precede
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ImportCommand.java:[90,33] (whitespace) WhitespaceAround: '|' is not followed by whitespace.
02:52:13  [ERROR] src/main/java/org/apache/zookeeper/cli/ImportCommand.java:[90,33] (whitespace) WhitespaceAround: '|' is not preceded with whitespace.

https://ci-hadoop.apache.org/job/zookeeper-precommit-github-pr/job/PR-2375/1/console

[ckrumbein]

PDavid, thanks for your comments. I just incorporated your feedback.

[PDavid]

Many thanks for these improvements. Looks good to me. 👍

[ckrumbein]

Sorry, I need some help. This is my first Apache PR, and my changes have been approved. I want to merge this change, but I don't see any merge button. Is it because there are 3 workflows awaiting approval? If so, it says this workflow requires approval from a maintainer.

[anmolnar]

So, basically these commands export and import the data of a single znode to and from a file. Why don't you just add this functionality to get and set commands?

[ckrumbein]

So, basically these commands export and import the data of a single znode to and from a file. Why don't you just add this functionality to get and set commands?

I could have done it that way; after all, I created the export and import commands by copying the get and set commands. However, I was trying to follow the lead of the ZKUI app, which has separate commands for viewing a znode and downloading a znode to a file. It's really just a matter of preference.

[ckrumbein]

Add to that, we've been using the export and import commands internally for some time, and we don't want to change!

[ckrumbein]

Andor Molnár, do you agree with my comments? Are you willing to approve this change, or is there anything else you'd like to discuss?

[anmolnar]

I approve the patch. Need another committer to merge it.

[ckrumbein]

Andor Molnár, I'm confused. In your last comment, you said I need another committer to merge. This change has already been approved by you and by Dávid Paksy. Are you saying I need a third person to approve this change to merge?

[anmolnar]

Andor Molnár, I'm confused. In your last comment, you said I need another committer to merge. This change has already been approved by you and by Dávid Paksy. Are you saying I need a third person to approve this change to merge?

@PDavid is not a committer. (yet)

[PDavid]

Andor Molnár, I'm confused. In your last comment, you said I need another committer to merge. This change has already been approved by you and by Dávid Paksy. Are you saying I need a third person to approve this change to merge?

@PDavid is not a committer. (yet)

Sorry for the confusion @ckrumbein. Indeed, I'm not a ZooKeeper committer, just a contributor for now.

[ckrumbein]

I see. How do I find another committer to approve my change? Is there a list of committers, or are there rules about how to reach out to them?

[ckrumbein]

I'm sorry for nagging about this. But I'm leaving my company at the end of this week, and it'll be very difficult for me to continue pursuing this after I leave.

[anmolnar]

You don't need to pursue I think. You can drop an e-mail to dev@ and/or user@ mailing lists to get some attention.

[ckrumbein]

I've now addressed the latest feedback from Andor Molnár and Christopher Tubbs.

[anmolnar]

still lgtm.

[ctubbsii]

To handle null values, you could have an export file format something like:

zk-data:len(array):array

This way, you can use a dash instead of a number in the length to indicate a null value.
If you don't want to bother storing the length, you can use a different punctuation to indicate a null, as in: zk-data$ for null and zk-data:data for non-null. Though, personally, I prefer storing the length.

Alternatively, if you only want to have the file contain the raw bytes, and no header or metadata, then you could just have an error if you try to export a null value, as in:

if (data == null) {
  throw new CliWrapperException(new IllegalArgumentException("Path contains no data to export"));
}

(Of course, this latter one would be most useful if there were a CLI command to set a path to null; currently, the only way to do that is in code; the only way to have a node set to null in the zkCli is to use the create command with no data argument, and there's no equivalent to set in the zkCli right now. If such a set option did exist, then this exception could be phrased to suggest using that.)

[ctubbsii]

This forces the command to treat null values the same as empty on import. This export command should store those differently, so it is clear on import which one it is.