You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/11/11 01:25:23 UTC
[GitHub] [zookeeper] mino181295 opened a new pull request #1532: ZOOKEEPER-3766 zkServer and other scripts should export CLASSPATH rather than use -cp
mino181295 opened a new pull request #1532:
URL: https://github.com/apache/zookeeper/pull/1532
## Motivation
ZooKeeper's launch scripts use `-cp` to pass the class path to Java when launching. This creates insanely large command-lines which are completely unnecessary.
Java respects the CLASSPATH environment variable, and this is how the class path should be passed to Java when the process launches.
So, instead of doing java -cp $CLASSPATH ..., the scripts should be doing export CLASSPATH; java ....
The long command-lines make it difficult to troubleshoot, or view running process lists in tools like top, htop, ps, but also make it impossible to search and manage using tools like pgrep and pkill, which can only search for the first 4096 characters in the command-line. (See https://github.com/apache/fluo-uno/issues/243#issuecomment-601553260 for a specific issue involving this limit caused by ZK's scripts.)
Master issue: [ZOOKEEPER-3766](https://issues.apache.org/jira/projects/ZOOKEEPER/issues/ZOOKEEPER-3766?filter=allopenissues)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on pull request #1532: ZOOKEEPER-3766 zkServer and other scripts should export CLASSPATH rather than use -cp
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1532:
URL: https://github.com/apache/zookeeper/pull/1532#issuecomment-725797052
> I don't think the test failing in the CI is related to my fix
The complete build is still very flaky, and has been for some time. However, the ZooKeeper project has an strong interest in ensuring that changes don't cause regressions. I agree that it does seem unrelated to your changes, but the nature of your changes being what they are, it's still *possible* that they could have affected the build in some way. You can try to force a rebuild by closing and reopening the PR, or by running `git commit --amend --reset-author` to update the timestamp on your commit message, and doing a force push.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] mino181295 commented on pull request #1532: ZOOKEEPER-3766 zkServer and other scripts should export CLASSPATH rather than use -cp
Posted by GitBox <gi...@apache.org>.
mino181295 commented on pull request #1532:
URL: https://github.com/apache/zookeeper/pull/1532#issuecomment-725321590
Thanks for the review and the help @ctubbsii
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on pull request #1532: ZOOKEEPER-3766 zkServer and other scripts should export CLASSPATH rather than use -cp
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1532:
URL: https://github.com/apache/zookeeper/pull/1532#issuecomment-740659957
Any update on this? This would be really helpful.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on a change in pull request #1532: ZOOKEEPER-3766 zkServer and other scripts should export CLASSPATH rather than use -cp
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1532:
URL: https://github.com/apache/zookeeper/pull/1532#discussion_r521784326
##########
File path: bin/zkEnv.sh
##########
@@ -120,6 +120,7 @@ for d in "$ZOOBINDIR"/../zookeeper-server/target/lib/*.jar
do
CLASSPATH="$d:$CLASSPATH"
done
+export CLASSPATH
Review comment:
Behaviorally, I don't think it matters, but stylistically, it always seemed to me that it would be best to export after making all assignments. There's a few lines below that still assign `CLASSPATH` to another value.
However, I don't think it matters, because I just did a quick test with two bash scripts, and it seems like if you export and then assign it a new value, the exported value is the new value, even if it was assigned after the export statement. I don't know if that works in all cases, though.
In any case, I still think it's good style to export after all assignments, rather than after some and before others.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] mino181295 commented on pull request #1532: ZOOKEEPER-3766 zkServer and other scripts should export CLASSPATH rather than use -cp
Posted by GitBox <gi...@apache.org>.
mino181295 commented on pull request #1532:
URL: https://github.com/apache/zookeeper/pull/1532#issuecomment-725685837
I don't think the test failing in the CI is related to my fix
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org