You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "soumitra-st (via GitHub)" <gi...@apache.org> on 2023/09/30 01:53:05 UTC
[GitHub] [zookeeper] soumitra-st opened a new pull request, #2071: Added options in get and set commands.
soumitra-st opened a new pull request, #2071:
URL: https://github.com/apache/zookeeper/pull/2071
get -f <output_file_name> to write into the file
set -f <input_file_name> to read the byte array from the file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added options in get and set commands. [zookeeper]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on PR #2071:
URL: https://github.com/apache/zookeeper/pull/2071#issuecomment-1760679065
Created JIRA - https://issues.apache.org/jira/browse/ZOOKEEPER-4760
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added options in get and set commands. [zookeeper]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on PR #2071:
URL: https://github.com/apache/zookeeper/pull/2071#issuecomment-1760686008
> Please create a Jira for this and describe the feature mentioning which version branch would you like it to be added. I think this is suitable for `master` and `branch-3.9` branches.
I could not find master and branch-3.9 in JIRA (https://issues.apache.org/jira/browse/ZOOKEEPER-4760), please let me know what else to do?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added options in get and set commands. [zookeeper]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #2071:
URL: https://github.com/apache/zookeeper/pull/2071#discussion_r1346765979
##########
zookeeper-server/src/main/java/org/apache/zookeeper/cli/GetCommand.java:
##########
@@ -93,10 +95,28 @@ public boolean exec() throws CliException {
throw new CliWrapperException(ex);
}
data = (data == null) ? "null".getBytes() : data;
- out.println(new String(data, UTF_8));
+
+ PrintStream output = out;
+
+ // Write to the file if specified
+ if (cl.hasOption("f")) {
+ String fileName = cl.getOptionValue("f");
+ try {
+ output = new PrintStream(fileName);
+ } catch (Exception ex) {
+ throw new CliException(ex);
+ }
+ }
+
+ output.println(new String(data, UTF_8));
Review Comment:
This prints a newline after the bytes, and also tries to interpret the bytes as UTF-8. If you're writing to a file, I don't think that's what it should be doing. It should just put the exact bytes it found, without trying to decode them as UTF-8. Also, this does not match what the set command is doing, which seems to be taking the file's exact bytes, and returning them directly, without any encoding or decoding, and without stripping out any trailing newline that this get command might have added.
Ultimately, I think it comes down to the fact that it really shouldn't be using a PrintStream at all, but should instead just do `Files.write(path, data)` (maybe opened with the `CREATE_NEW` option so it doesn't clobber an existing file accidentally). `Files.write` is the opposite of the `Files.readAllBytes` that is used in the corresponding set command in this PR. `println(new String())` is definitely not.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added options in get and set commands. [zookeeper]
Posted by "soumitra-st (via GitHub)" <gi...@apache.org>.
soumitra-st commented on code in PR #2071:
URL: https://github.com/apache/zookeeper/pull/2071#discussion_r1349609808
##########
zookeeper-docs/src/main/resources/markdown/zookeeperCLI.md:
##########
@@ -57,7 +57,7 @@ ZooKeeper -server host:port cmd args
reconfig [-s] [-v version] [[-file path] | [-members serverID=host:port1:port2;port3[,...]*]] | [-add serverId=host:port1:port2;port3[,...]]* [-remove serverId[,...]*]
redo cmdno
removewatches path [-c|-d|-a] [-l]
- set [-s] [-v version] path data
+ set [-s] [-v version] path data [-f input_file_name]
Review Comment:
data and -f both options together are not valid, added check and test to cover that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added options in get and set commands. [zookeeper]
Posted by "walterddr (via GitHub)" <gi...@apache.org>.
walterddr commented on code in PR #2071:
URL: https://github.com/apache/zookeeper/pull/2071#discussion_r1344342452
##########
zookeeper-docs/src/main/resources/markdown/zookeeperCLI.md:
##########
@@ -57,7 +57,7 @@ ZooKeeper -server host:port cmd args
reconfig [-s] [-v version] [[-file path] | [-members serverID=host:port1:port2;port3[,...]*]] | [-add serverId=host:port1:port2;port3[,...]]* [-remove serverId[,...]*]
redo cmdno
removewatches path [-c|-d|-a] [-l]
- set [-s] [-v version] path data
+ set [-s] [-v version] path data [-f input_file_name]
Review Comment:
shouldn't it be
```suggestion
set [-s] [-v version] path [-d data] [-f input_file_name]
```
? what does it mean when both data and input file are provided?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4760. Added options in get and set commands. [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2071:
URL: https://github.com/apache/zookeeper/pull/2071#issuecomment-1762735097
> > Please create a Jira for this and describe the feature mentioning which version branch would you like it to be added. I think this is suitable for `master` and `branch-3.9` branches.
>
> I could not find master and branch-3.9 in JIRA (https://issues.apache.org/jira/browse/ZOOKEEPER-4760), please let me know what else to do?
You should only set the fix versions to the Jira where you would like to see the fix. Currently master branch is for 3.10.0 and branch-3.9 is the upcoming, 3.9.2 release.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org