You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/10/26 13:24:39 UTC
[GitHub] [cassandra] Claudenw opened a new pull request, #1950: WIP: Initial implementation of cassandra-conf with nodetool example
Claudenw opened a new pull request, #1950:
URL: https://github.com/apache/cassandra/pull/1950
This is a work in progress I am looking for comments
Thanks for sending a pull request! Here are some tips if you're new here:
* Ensure you have added or run the [appropriate tests](https://cassandra.apache.org/_/development/testing.html) for your PR.
* Be sure to keep the PR description updated to reflect all changes.
* Write your PR title to summarize what this PR proposes.
* If possible, provide a concise example to reproduce the issue for a faster review.
* Read our [contributor guidelines](https://cassandra.apache.org/_/development/index.html)
* If you're making a documentation change, see our [guide to documentation contribution](https://cassandra.apache.org/_/development/documentation.html)
Commit messages should follow the following format:
```
<One sentence description, usually Jira title or CHANGES.txt summary>
<Optional lengthier description (context on patch)>
patch by Claude Warren; reviewed by <Reviewers> for CASSANDRA-17773
Co-authored-by: Name1 <email1>
Co-authored-by: Name2 <email2>
```
The [Cassandra Jira](https://issues.apache.org/jira/projects/CASSANDRA/issues/)
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094591341
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
CASSANDRA_CONF is an existing environment variable, it is used to load cassendra-env.sh. which I believe is a user editable file for configurations. `cassandra-conf.sh` now strikes me as a very bad name as it is not really associated with CASSANDRA_CONF. `cassandra-conf.sh`can not be dependant upon CASSANDRA_CONF as CASSANDRA_CONF may be set using `cassandra.in.sh` or whatever CASSANDRA_INCLUDE points to.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1113157425
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
since `cassandra-conf.sh` is intended to be used "as is" without modification I think it should be referenced in-situ. If one is writing a new script to source `cassandra-conf.sh` then the full path should be provided. Any calculation of the location of `cassandra-conf.sh` is, in my mind, risky and potentially hard to diagnose when an error occurs.
The text:
```
if [ -z $CASSANDRA_HOME ]; then
. $(dirname "$0")/../conf/cassandra-conf.sh
else
. $CASSANDRA_HOME/conf/cassandra-conf.sh
fi
```
won't work because CASSANDRA_HOME is not expected to be set until after `cassandra.in.sh` which is loaded by `cassandra-conf.sh`, also the `cassandra-conf.sh` file is expected to be in `bin/` not `conf/`. But even if `bin/` were used it it would still be incorrect for any new script that was not in a sibling directory of `bin/`.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1111978614
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
I tend to disagree how this is implemented but I do need to test this manually to see how this really behaves.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112835095
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
Imagine you are developing some 3rd party command-line tool which needs the very same boilerplate as any other command we ship in Cassandra distribution, in `tools` or `bin` or what have you.
While implementing the script which executes that main class of yours, you basically take some already existing script as a skeleton and then you tweak it in such a way that the java command at the end of that file executes your main class.
When we eventually plan to incorporate this `cassandra-conf.sh` stuff into every script, one can imagine that the very same script will be sourced in our custom script as well.
Here, we are making an assumption that `cassandra-conf.sh` will be in the same directory as `nodetool` script is.
But what if it is not? What if you have some 3rd party script which is located it a different directory from nodetool's one? What if that script is in `/tmp/mycustomscripts/mytool` ? Then sourcing `. cassandra-conf.sh` will not work, will it?
So for that reason, the loading of `. cassandra-conf.sh` has to be a little bit smarter about that and we should source it in such a way that the script it is sourced in can be in theory wherever. That is what my suggestion about sourcing with above is about.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1446230130
If CASSANDRA_HOME were set in every case then there would be no need for `cassandra.in.sh` to set it, and yet it does.
`cassandra-conf.sh` can not depend upon CASSANDRA_HOME being set.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1445962680
@michaelsembwever
1. I agree the name is confusing, does anyone have a better name? The goal of this change is to simply remove the boilerplate from the other scripts while providing debugging capabilities to more easily detect the issue that caused CASSANDRA-17773. I did not make any change to operation of the scripts. Pulling a `cassandra-env.conf` out of `cassandra-env.sh` may be a reasonable thing to do but is outside the scope of this change.
2. Script is silent by default. I think you are referring to the `-x` argument. That was included in error and was a debugging switch.
3. This script has to run before before we can assume any environment variables are set as it sets many of them through the logic that pulls in the cassandra.in.sh script. I make 2 assumptions for Cassandra provided scripts.
* The scripts in `'bin/` are not to be edited by end users
* The scripts provided by Cassandra are all deployed in `bin/`. Thus the `cassandra-conf.sh` will be in the same directory as `nodetool`. However, I realise that there was an error in the way the file was included. The code in nodetool now uses `realpath` and `dirname` to figure out where `cassandra-conf.sh` is. This means that symbolic links to `nodetool` will work correctly. (other cassandra provided scripts in `bin/` will be configured the same way)
For user created scripts I assume they know where `bin/` is and will modify their scripts to source `cassandra-conf.sh` from there.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052018501
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
+ fi
+
+ while [ -n "$1" ] ; do
+ eval echo "$1=\$$1"
+ shift
+ done
+
+ if [ -n "$ENV_SHOW" ]
Review Comment:
should not this be explicitly equal to `1` or `yes` ?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052010402
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
Review Comment:
"_creates_"?
criticla -> critical
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1613085038
1)
where did we get with this? @smiklosovic seems to say you agree, while you say it is out of scope ?
3)
If `CASSANDRA_HOME` is set it must be honoured.
One example I know of, it needs to be possible to execute scripts in a random place against a CASSANDRA_HOME somewhere else. (tests do this)
--
otherwise… my concerns with this work is that we it requires a lot of eyes and testing, and that we won't get that before the August freeze.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112835095
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
Imagine you are developing some 3rd party command-line tool which needs the very same boilerplate as any other command we ship in Cassandra distribution, in `tools` or `bin` or what have you.
While implementing the script which executes that main class of yours, you basically take some already existing script as a skeleton and then you tweak it in such a way that the java command at the end of that file executes your main class.
When we eventually plan to incorporate this `cassandra-conf.sh` stuff into every script, one can imagine that the very same script will be called in our custom script as well.
Here, we are making an assumption that `cassandra-conf.sh` will be in the same directory as `nodetool` script is.
But what it is not? What if you have some 3rd party script which is located it a different directory from nodetool's one? What if that script is in `/tmp/mycustomscripts/mytool` ? Then sourcing `. cassandra-conf.sh` will not work, will it?
So for that reason, the loading of `. cassandra-conf.sh` has to be a little bit smarter about that we should souce it in such a way that the script it is sourced in can be in theory wherever. That is what my suggestion about sourcing with above is about.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094550041
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
````
if [ -z $CASSANDRA_HOME ]; then
. $(dirname "$0")/../conf/cassandra-conf.sh
else
. $CASSANDRA_HOME/conf/cassandra-conf.sh
fi
````
This should enable people to execute it without having `CASSANDRA_HOME` set. I think nobody is going to move nodetool script away from its current location so it is given that conf file will be in `../conf/cassanra-conf.sh` from `nodetool`'s perspective.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052019266
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
+ fi
+
+ while [ -n "$1" ] ; do
+ eval echo "$1=\$$1"
Review Comment:
what about printing these variables too? Not sure if it is secure as it may potentially contain credentials but I dont think vanilla Cassandra does 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112053251
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
`cassandra_conf.sh` pulls the common code from all the existing scripts. This is found at:
https://github.com/apache/cassandra/pull/1950/files#diff-61e866313476b2b21fa3d88d315567447c43de4f55066776bc649d9f898b3bf6R105-R140
also lines 151-153 and 165-167
nodetool then has special requirements to unset some vars so it needs to preserve and reset some around the cassandra-env.sh call.
the script `cassandra.in.sh`
- can move and is located by various means.
- sets CASSANDRA_HOME and CASSANDRA_CONF if not set.
- sets JAVA_AGENT, CLASSPATH, JVM_OPTS, JVM_DEP_OPTS, and JAVA_VERSION
- is a file that may be edited by users to change the configuration.
so `cassandra_conf.sh` should not move. It should always be in the same directory with the script(s) loading it.
any script using `cassandra_conf.sh` will have the lines
```. ./cassandra-conf.sh
validate_env
```
some, like `nodetool` my preserve vars and so set `CASSANDRA_ENV_PRESERVE` with a list of variables to preserve.
some like `nodetool` may set additional vars for which debugging display is appropriate. Those will list the additional env vars after the `validate_env` as `nodetool` does
```
validate_env JMX_PORT
```
Simply put `cassandra_conf.sh` removes the boilerplate from the scripts to create one place where all the configuration determination is performed, it has hooks to support current operations (like `nodetool`), it adds debugging of the environment variables to make problem determination easier and consistent across all the script based tooling.
I hope this overview helps with your testing.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112053251
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
`cassandra_conf.sh` pulls the common code from all the existing scripts. This is found at:
https://github.com/apache/cassandra/pull/1950/files#diff-61e866313476b2b21fa3d88d315567447c43de4f55066776bc649d9f898b3bf6R105-R140
also lines 151-153 and 165-167
nodetool then has special requirements to unset some vars so it needs to preserve and reset some around the cassandra-env.sh call.
the script `cassandra.in.sh`
- can move and is located by various means.
- sets CASSANDRA_HOME and CASSANDRA_CONF if not set.
- sets JAVA_AGENT, CLASSPATH, JVM_OPTS, JVM_DEP_OPTS, and JAVA_VERSION
- is a file that may be edited by users to change the configuration.
so `cassandra_conf.sh` should not move. It should always be in the same directory with the script(s) loading it.
any script using `cassandra_conf.sh` will have the lines
```. ./cassandra-conf.sh
validate_env
```
some, like `nodetool` my preserve vars and so set `CASSANDRA_ENV_PRESERVE` with a list of variables to preserve.
some like `nodetool` may set additional vars for which debugging display is appropriate. Those will list the additional env vars after the `validate_env` as `nodetool` does
```
validate_env JMX_PORT
```
Simply put `cassandra_conf.sh` removes the boilerplate from the scripts to create one place where all the configuration determination is performed, it has hooks to support current operations (like `nodetool`), it adds debugging of the environment variables to make problem determination easier and consistent across all the script based tooling.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052014065
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
Review Comment:
does not `sh` have something like `elif` ? So you can make this less verbose and save some lines.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052012540
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
Review Comment:
`ENV_PRESERVE` is missing
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1494294437
@smiklosovic , @michaelsembwever any further comments on this?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094591341
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
CASSANDRA_CONF is an existing environment variable, it is used to load cassendra-env.sh. which I believe is a user editable file for configurations. `cassandra-conf.sh` now strikes me as a very bad name as it is not really associated with CASSANDRA_CONF. `cassandra-conf.sh`can not be dependant upon CASSANDRA_CONF as CASSANDRA_CONF may be set using `cassandra.in.sh` or whatever CASSANDRA_INCLUDE points to.
The line in question here points to the cassandra-conf.sh which is in the same directory as nodetool.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1081869918
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
+ fi
+
+ while [ -n "$1" ] ; do
+ eval echo "$1=\$$1"
+ shift
+ done
+
+ if [ -n "$ENV_SHOW" ]
Review Comment:
I understand, but I think "to be set" is not enough. When somebody sets it to "fals" when they meant "false" and they made typo, this will evaluate it like "it is set" and it will be shown .... I am for explicitly setting this stuff every time.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052011623
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
Review Comment:
maybe having them prefixed so it would be like `CASSANDRA_ENV_DEBUG` and similar is better? A variable called "ENV_SHOW" is quite non-telling if I just execute `$ env` command. I do not know what software that environment variable relates to.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1113948104
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
_won't work because CASSANDRA_HOME is not expected to be set until after cassandra.in.sh which is loaded by cassandra-conf.sh_
Is this really the case? I am pretty much used to have the environment property `CASSANDRA_HOME` set way before I start Cassandra / execute any script. It is set in my environment variables in .bashrc / .bash_profile or so ...
We would just move this
https://github.com/apache/cassandra/blob/trunk/bin/cassandra.in.sh#L17-L19
to the script itself.
I would not hurt if we left it in cassandra.in.sh anyway. It would just do nothing.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094546931
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,167 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Defines a validate_env() function to verify critical environment vars are set
+# and display them if desired
+
+## Environment variables:
+# CASSANDRA_ENV_DEBUG: Show all var settings during the validate_env() call.
+# CASSANDRA_ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# CASSANDRA_ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## CASSANDRA_ENV_DEBUG Enables display of variables if this var is set.
+## CASSANDRA_ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$CASSANDRA_ENV_DEBUG" ] ; then
+ echo ""
Review Comment:
@Claudenw I dont think this is necessary. Same empty line after the closing "------" is redundant.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094547468
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,167 @@
+#!/bin/sh -x
Review Comment:
@Claudenw is "-x" necessary?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052016328
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
Review Comment:
should not this be explicitly equal to `1` or `yes` ?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052022938
##########
bin/nodetool:
##########
@@ -98,6 +71,8 @@ if [ "x$MAX_HEAP_SIZE" = "x" ]; then
MAX_HEAP_SIZE="128m"
fi
+validate_env JMX_PORT
Review Comment:
could you please remind me why we are checking just JMX_PORT here? Seems to be quite random but I guess it has some logic in it?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1445186486
I fully agree with the third point, @michaelsembwever . That is what I was trying to communicate as well. I think that Claude reached the same conclusion about 1) and I dont have any objections with 2), definitely it should be silent when executed just fine to follow basic *nix principles.
Not sure about your last question. Maybe Claude can try to model that, just for the sake of seeing how that would look like.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1609407780
@smiklosovic , @michaelsembwever any further comments on this or should I go ahead an perform the changes on all the scripts?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1613485675
@Claudenw I would rather focus on this PR bellow, we need to be smart about time in the context of incoming freeze. That one is a new feature, this is "just" an improvement. AFAIK new features in a freeze window are forbidden.
https://github.com/apache/cassandra/pull/2282
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094583556
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,167 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Defines a validate_env() function to verify critical environment vars are set
+# and display them if desired
+
+## Environment variables:
+# CASSANDRA_ENV_DEBUG: Show all var settings during the validate_env() call.
+# CASSANDRA_ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# CASSANDRA_ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## CASSANDRA_ENV_DEBUG Enables display of variables if this var is set.
+## CASSANDRA_ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$CASSANDRA_ENV_DEBUG" ] ; then
+ echo ""
Review Comment:
It makes a clear separation in the logs
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094582779
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,167 @@
+#!/bin/sh -x
Review Comment:
no, it is for testing. It should be removed before merge.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094559887
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
This should also somehow honor `CASSANDRA_CONF` property if it is set.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1094592713
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
Review Comment:
This should read `CASSANDRA_ENV_PRESERVE="$CASSANDRA_ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1081312367
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
Review Comment:
Added CASSANDRA_ before all ENV_ e.g. CASSANDRA_ENV_PRESERVE
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
Review Comment:
changed to define and fixed typo.
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
Review Comment:
The documentation says if it is set. Doesn't say anything about value. I suppose it could be changed.
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
Review Comment:
added
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
Review Comment:
This is not an error, it is a message that only is displayed while debugging. Not having it set was not an error before.
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
+ fi
+
+ while [ -n "$1" ] ; do
+ eval echo "$1=\$$1"
+ shift
+ done
+
+ if [ -n "$ENV_SHOW" ]
+ then
Review Comment:
fixed
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
+ fi
+
+ while [ -n "$1" ] ; do
+ eval echo "$1=\$$1"
+ shift
+ done
+
+ if [ -n "$ENV_SHOW" ]
Review Comment:
The CASSANDRA_ENV_SHOW is a list of variables to show so if it is set then we use it, if it is not set we don't display anything,
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
Review Comment:
It could, but this solution lists all the missing symbols in one run.
##########
bin/nodetool:
##########
@@ -98,6 +71,8 @@ if [ "x$MAX_HEAP_SIZE" = "x" ]; then
MAX_HEAP_SIZE="128m"
fi
+validate_env JMX_PORT
Review Comment:
It is not checking just the JMX_PORT, however when debugging the JMX_PORT will be displayed, this is added because the JMX_PORT can be set above.
Added a comment reflecting this.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1111976709
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,167 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Defines a validate_env() function to verify critical environment vars are set
+# and display them if desired
+
+## Environment variables:
+# CASSANDRA_ENV_DEBUG: Show all var settings during the validate_env() call.
+# CASSANDRA_ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# CASSANDRA_ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## CASSANDRA_ENV_DEBUG Enables display of variables if this var is set.
+## CASSANDRA_ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$CASSANDRA_ENV_DEBUG" ] ; then
+ echo ""
Review Comment:
Dont we have "-----------" for separation already? What do we have "--------" for if we still need to separate it by an empty line?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] michaelsembwever commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "michaelsembwever (via GitHub)" <gi...@apache.org>.
michaelsembwever commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1445011660
Some questions…
1. isn't this a wrapper to either conf/cassandra-env.sh or bin/cassandra.in.sh ? the name cassandra-conf.sh is confusing…
2. scripts should be silent when successful, by default
3. you can't presume the file is in the same directory, or where anything is being executed from,
wrt (1), couldn't we instead pull a cassandra-env.conf out of cassandra-env.sh so to split the logic and the user-editable variables into separate files …?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1446119787
I yet again want to reiterate the fact that environment property `CASSANDRA_HOME` might be set in user's environment and it is in fact set way before any script is executed so the third point is not entirely true.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112835095
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
Imagine you are developing some 3rd party command-line tool which needs the very same boilerplate as any other command we ship in Cassandra distribution, in `tools` or `bin` or what have you.
While implementing the script which executes that main class of yours, you basically take some already existing script as a skeleton and then you tweak it in such a way that the java command at the end of that file executes your main class.
When we eventually plan to incorporate this cassandra-conf.sh stuff into every script, one can imagine that the very same script will be called in our custom script as well.
Here, we are making an assumption that `cassandra-conf.sh` will be in the same directory as `nodetool` script is.
But what it is not? What if you have some 3rd party script which is located it a different directory from nodetool's one? What if that script is in `/tmp/mycustomscripts/mytool` ? Then sourcing `. cassandra-conf.sh` will not work, will it?
So for that reason, the loading of `. cassandra-conf.sh` has to be a little bit smarter about that we should souce it in such a way that the script it is sourced in can be in theory wherever. That is what my suggestion about sourcing with above is about.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112835095
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
Imagine you are developing some 3rd party command line tool which needs the very same boiler plate as any other command we ship in Cassandra distribution, in `tools` or `bin` or what have you.
While implementing the script which executes that main class of yours, you basically take some already existing script as a skeleton and then you tweak it in such a way that the java command at the end of that file executes your main class.
When we eventually plan to incorporate this cassandra-conf.sh stuff into every script, one can imagine that the very same script will be called in our custom script as well.
Here, we are making an assumption that `cassandra-conf.sh` will be in the same directory as `nodetool` script is.
But what it is not? What if you have some 3rd party script which is located it a different directory from nodetool's one? What if that script is in `/tmp/mycustomscripts/mytool` ? Then sourcing `. cassandra-conf.sh` will not work, will it?
So for that reason, the loading of `. cassandra-conf.sh` has to be a little bit smarter about that we should souce it in such a way that the script it is sourced in can be in theory wherever. That is what my suggestion about sourcing with above is about.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052020200
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
+ fi
+
+ while [ -n "$1" ] ; do
+ eval echo "$1=\$$1"
+ shift
+ done
+
+ if [ -n "$ENV_SHOW" ]
+ then
Review Comment:
once you put `then` after `if` at the same line, next time it is on new line like here, it should be consistent.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1052020200
##########
bin/cassandra-conf.sh:
##########
@@ -0,0 +1,168 @@
+#!/bin/sh -x
+
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+# A script to include the cassandra.in.sh file, set NUMACTL, and load
+# $CASSANDRA_CONF/cassandra-env.sh"
+#
+# Creates a validate_env() function to verify criticla environment vars are set
+# and display them if desired
+
+## Environment variables:
+# ENV_DEBUG: Show all var settings during the validate_env() call.
+# ENV_PRESERVE: A list of environment variables to preserve from being set
+# by $CASSANDRA_CONF/cassandra-env.sh"
+# ENV_SHOW: A list of additional environment variables to display during validate_env.
+
+## validate_env
+## Args: Arguments are additional environment variables to show.
+##
+## Validates that required environment variables are set.
+## Required Environment Vars:
+## CASSANDRA_INCLUDE
+## CASSANDRA_HOME
+## CASSANDRA_LOG_DIR
+##
+## Environment Vars:
+## ENV_DEBUG Enables display of variables.
+## ENV_SHOW List of additional variables to display.
+##
+validate_env() {
+ retval=0
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo ""
+ echo "ENVIRONMENT"
+ echo "-----------"
+ echo "PWD=$PWD"
+ echo "CASSANDRA_CONF=$CASSANDRA_CONF"
+ fi
+
+ if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
+ echo ">>> cassandra.in.sh not found or CASSANDRA_INCLUDE not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_INCLUDE=$CASSANDRA_INCLUDE"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_HOME" = "x" ]; then
+ echo ">>> CASSANDRA_HOME not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_HOME=$CASSANDRA_HOME"
+ fi
+ fi
+
+ if [ "x$CASSANDRA_LOG_DIR" = "x" ]; then
+ echo ">>> CASSANDRA_LOG_DIR not set" >&2
+ retval=1
+ else
+ if [ -n "$ENV_DEBUG" ] ; then
+ echo "CASSANDRA_LOG_DIR=$CASSANDRA_LOG_DIR"
+ fi
+ fi
+
+ if [ -n "$ENV_DEBUG" ] ; then
+ if [ -z $NUMACTL ]; then
+ echo "numactl not found"
+ fi
+
+ while [ -n "$1" ] ; do
+ eval echo "$1=\$$1"
+ shift
+ done
+
+ if [ -n "$ENV_SHOW" ]
+ then
Review Comment:
once you put `then' after `if` at the same line, next time it is on new line like here, it should be consistent.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by GitBox <gi...@apache.org>.
Claudenw commented on PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#issuecomment-1307178327
@mmuzaf You may be interested in this change.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] Claudenw commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "Claudenw (via GitHub)" <gi...@apache.org>.
Claudenw commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1115434174
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
One of the goals of this change is to move the boiler plate to a single location and call it. It should not require a change is step order of an existing file (e.g. defining CASSANDRA_HOME earlier or even explicitly)
If someone writing a new script wants to source `cassandra_conf.sh` from a known CASSANDRA_HOME that is fine, but I don't think we should require it of the existing scripts that do not currently set CASSANDRA_HOME and that only check the value of CASSANDRA_HOME after `cassandra.in.sh` is called.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1113948104
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
_won't work because CASSANDRA_HOME is not expected to be set until after cassandra.in.sh which is loaded by cassandra-conf.sh_
Is this really the case? I am pretty much used to have the environment property `CASSANDRA_HOME` set way before I start Cassandra / execute any script. It is set in my environment variables in .bashrc / .bash_profile or so ...
We would just move this
https://github.com/apache/cassandra/blob/trunk/bin/cassandra.in.sh#L17-L19
to the script itself. That logic just sets CASSANDRA_HOME when it is not set. But if I set it before I execute any script, it will take it into account.
It would not hurt if we left it in `cassandra.in.sh` anyway. It would just do nothing if we happen to set it in the script itself already.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112835095
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
Imagine you are developing some 3rd party command line tool which needs the very same boilerplate as any other command we ship in Cassandra distribution, in `tools` or `bin` or what have you.
While implementing the script which executes that main class of yours, you basically take some already existing script as a skeleton and then you tweak it in such a way that the java command at the end of that file executes your main class.
When we eventually plan to incorporate this cassandra-conf.sh stuff into every script, one can imagine that the very same script will be called in our custom script as well.
Here, we are making an assumption that `cassandra-conf.sh` will be in the same directory as `nodetool` script is.
But what it is not? What if you have some 3rd party script which is located it a different directory from nodetool's one? What if that script is in `/tmp/mycustomscripts/mytool` ? Then sourcing `. cassandra-conf.sh` will not work, will it?
So for that reason, the loading of `. cassandra-conf.sh` has to be a little bit smarter about that we should souce it in such a way that the script it is sourced in can be in theory wherever. That is what my suggestion about sourcing with above is about.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112835095
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
Imagine you are developing some 3rd party command-line tool which needs the very same boilerplate as any other command we ship in Cassandra distribution, in `tools` or `bin` or what have you.
While implementing the script which executes that main class of yours, you basically take some already existing script as a skeleton and then you tweak it in such a way that the java command at the end of that file executes your main class.
When we eventually plan to incorporate this `cassandra-conf.sh` stuff into every script, one can imagine that the very same script will be sourced in our custom script as well.
Here, we are making an assumption that `cassandra-conf.sh` will be in the same directory as `nodetool` script is.
But what it is not? What if you have some 3rd party script which is located it a different directory from nodetool's one? What if that script is in `/tmp/mycustomscripts/mytool` ? Then sourcing `. cassandra-conf.sh` will not work, will it?
So for that reason, the loading of `. cassandra-conf.sh` has to be a little bit smarter about that we should souce it in such a way that the script it is sourced in can be in theory wherever. That is what my suggestion about sourcing with above is about.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
[GitHub] [cassandra] smiklosovic commented on a diff in pull request #1950: WIP: Initial implementation of cassandra-conf with nodetool example - CASSANDRA-17773
Posted by "smiklosovic (via GitHub)" <gi...@apache.org>.
smiklosovic commented on code in PR #1950:
URL: https://github.com/apache/cassandra/pull/1950#discussion_r1112835095
##########
bin/nodetool:
##########
@@ -22,35 +22,8 @@ if [ "`basename "$0"`" = 'nodeprobe' ]; then
echo "***************************************************************" >&2
fi
-if [ "x$CASSANDRA_INCLUDE" = "x" ]; then
- # Locations (in order) to use when searching for an include file.
- for include in "`dirname "$0"`/cassandra.in.sh" \
- "$HOME/.cassandra.in.sh" \
- /usr/share/cassandra/cassandra.in.sh \
- /usr/local/share/cassandra/cassandra.in.sh \
- /opt/cassandra/cassandra.in.sh; do
- if [ -r "$include" ]; then
- . "$include"
- break
- fi
- done
-elif [ -r "$CASSANDRA_INCLUDE" ]; then
- . "$CASSANDRA_INCLUDE"
-fi
-
-if [ -z "$CASSANDRA_CONF" -o -z "$CLASSPATH" ]; then
- echo "You must set the CASSANDRA_CONF and CLASSPATH vars" >&2
- exit 1
-fi
-
-# Run cassandra-env.sh to pick up JMX_PORT
-if [ -f "$CASSANDRA_CONF/cassandra-env.sh" ]; then
- JVM_OPTS_SAVE=$JVM_OPTS
- MAX_HEAP_SIZE_SAVE=$MAX_HEAP_SIZE
- . "$CASSANDRA_CONF/cassandra-env.sh"
- MAX_HEAP_SIZE=$MAX_HEAP_SIZE_SAVE
- JVM_OPTS="$JVM_OPTS_SAVE"
-fi
+ENV_PRESERVE="$ENV_PRESERVE MAX_HEAP_SIZE JVM_OPTS"
+. ./cassandra-conf.sh
Review Comment:
Imagine you are developing some 3rd party command-line tool which needs the very same boilerplate as any other command we ship in Cassandra distribution, in `tools` or `bin` or what have you.
While implementing the script which executes that main class of yours, you basically take some already existing script as a skeleton and then you tweak it in such a way that the java command at the end of that file executes your main class.
When we eventually plan to incorporate this `cassandra-conf.sh` stuff into every script, one can imagine that the very same script will be sourced in our custom script as well.
Here, we are making an assumption that `cassandra-conf.sh` will be in the same directory as `nodetool` script is.
But what if it is not? What if you have some 3rd party script which is located it a different directory from nodetool's one? What if that script is in `/tmp/mycustomscripts/mytool` ? Then sourcing `. cassandra-conf.sh` will not work, will it?
So for that reason, the loading of `. cassandra-conf.sh` has to be a little bit smarter about that we should souce it in such a way that the script it is sourced in can be in theory wherever. That is what my suggestion about sourcing with above is about.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org