Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
jerryshao merged PR #5977: URL: https://github.com/apache/gravitino/pull/5977 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1912847452 ## bin/common.sh: ## Review Comment: @xunliu , thank you for the feedback. I have updated it as suggested. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
xunliu commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1912831446 ## bin/common.sh: ## Review Comment: hi @liuchunhao I think maybe we can modify this file suffix to `.sh.template`, keep it 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
tengqm commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1912390289
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "GRAVITINO_VERSION is not set, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
Review Comment:
Alright. My bad actually. Shell operates differently from Python.
No extra spaces are needed in this case.
Please help revert 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
tengqm commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1912390289
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "GRAVITINO_VERSION is not set, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
Review Comment:
Alright. My bad actually. Shell operates differently from Python.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1912342954
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "GRAVITINO_VERSION is not set, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
Review Comment:
Thank you for your feedback. After testing, I found that if a single space
is added at the end of each line, the final output actually contains two
spaces. This happens because there is already a space between the backslash `\`
and the content, and the indentation at the beginning of each line also
contributes to the whitespace in the output.
I have added spaces based on your suggestions.
If I have misunderstood your intentions regarding adding whitespace at the
end of each line, kindly let me know.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
tengqm commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1911957907
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "GRAVITINO_VERSION is not set, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
Review Comment:
```suggestion
"1. Ensure that a compiled version of Gravitino is available at " \
```
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "GRAVITINO_VERSION is not set, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
+ "\${GRAVITINO_HOME}/distribution/package. You may need to compile it first,"
\
Review Comment:
```suggestion
"\${GRAVITINO_HOME}/distribution/package. You may need to compile it
first, " \
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1911892188
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No variable GRAVITINO_VERSION was found, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
+ "/distribution/package. If not, you must compile it
first.\n" \
Review Comment:
Thanks for the suggestion. I've updated it accordingly
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1911879329
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No variable GRAVITINO_VERSION was found, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
+ "/distribution/package. If not, you must compile it
first.\n" \
+ "2. Execute gravitino.sh from /distribution/package/bin" \
+ "and avoid running scripts from other directories."
Review Comment:
This is because many developers try to run Gravitino using
${GRAVITINO}/bin/gravitino.sh. The error message is intended to guide them to
use the correct script: ${GRAVITINO}/distribution/package/bin/gravitino.sh.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1911876909 ## docs/getting-started.md: ## @@ -176,7 +176,7 @@ To use Gravitino locally on macOS or Linux, follow these similar steps: Or, you can install Gravitino from scratch, follow [how-to-build](./how-to-build.md) and [how-to-install](./how-to-install.md). -3. Start Gravitino using the gravitino.sh script: +3. Start Gravitino using the gravitino.sh script in the binary release package or Docker image: ```shell /bin/gravitino.sh start Review Comment: Thank you for the feedback. The update has been made as suggested -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1911874044
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No variable GRAVITINO_VERSION was found, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
+ "/distribution/package. If not, you must compile it
first.\n" \
+ "2. Execute gravitino.sh from /distribution/package/bin" \
Review Comment:
Thank you for the feedback. The update has been made as suggested
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
tengqm commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1911870664
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No variable GRAVITINO_VERSION was found, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
+ "/distribution/package. If not, you must compile it
first.\n" \
Review Comment:
```suggestion
"\${GRAVITINO_HOME}/distribution/package. You may need to compile it first
" \
"if you are installing the software from source code.\n" \
```
##
docs/getting-started.md:
##
@@ -176,7 +176,7 @@ To use Gravitino locally on macOS or Linux, follow these
similar steps:
Or, you can install Gravitino from scratch, follow
[how-to-build](./how-to-build.md) and [how-to-install](./how-to-install.md).
-3. Start Gravitino using the gravitino.sh script:
+3. Start Gravitino using the gravitino.sh script in the binary release package
or Docker image:
```shell
/bin/gravitino.sh start
Review Comment:
`path-to-gravitino` is not a professional way to communicate to users.
We can mimic the Java way of running commands. That is, we need a
`GRAVITINO_HOME`
environment variable to run the commands/scripts.
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No variable GRAVITINO_VERSION was found, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
+ "/distribution/package. If not, you must compile it
first.\n" \
+ "2. Execute gravitino.sh from /distribution/package/bin" \
Review Comment:
```suggestion
"2. Execute gravitino.sh in the
\${GRAVITINO_HOME}/distribution/package/bin directory."
```
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No variable GRAVITINO_VERSION was found, you may need to:\n" \
Review Comment:
```suggestion
echo -e "GRAVITINO_VERSION is not set, you may need to:\n" \
```
##
bin/common.sh:
##
@@ -42,6 +42,15 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No variable GRAVITINO_VERSION was found, you may need to:\n" \
+ "1. Ensure that a compiled version of Gravitino is available at" \
+ "/distribution/package. If not, you must compile it
first.\n" \
+ "2. Execute gravitino.sh from /distribution/package/bin" \
+ "and avoid running scripts from other directories."
Review Comment:
Not sure why we want to mention this.
If the scripts cannot be executed from other directory, we need to improve
the them.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1911869742
##
bin/common.sh:
##
@@ -42,6 +42,11 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No GRAVITINO_VERSION was found, you may need to:\n1. Run the
gravitino.sh script on the compiled Gravitino.\n2. Run the gravitino.sh script
in the release package."
Review Comment:
I have made an update to address the issue of lines being too long and to
provide clearer instruction messages about errors. Please let me know if you
have any suggestions
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
jerryshao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1909779515
##
bin/common.sh:
##
@@ -42,6 +42,11 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No GRAVITINO_VERSION was found, you may need to:\n1. Run the
gravitino.sh script on the compiled Gravitino.\n2. Run the gravitino.sh script
in the release package."
Review Comment:
What I mean is to break this long line into several lines in this code, not
adding `\n` for the output..
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
justinmclean commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1909621646
##
bin/common.sh:
##
@@ -42,6 +42,11 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No GRAVITINO_VERSION was found, you may need to:\n1. Run the
gravitino.sh script on the compiled Gravitino.\n2. Run the gravitino.sh script
in the release package."
Review Comment:
Also, the release package is a source package, so the instructions are
slightly misleading.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1909588807
##
bin/common.sh:
##
@@ -42,6 +42,11 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No GRAVITINO_VERSION was found, you may need to:\n1. Run the
gravitino.sh script on the compiled Gravitino.\n2. Run the gravitino.sh script
in the release package."
Review Comment:
Sure, no problem
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1909582472 ## docs/getting-started.md: ## @@ -176,11 +176,17 @@ To use Gravitino locally on macOS or Linux, follow these similar steps: Or, you can install Gravitino from scratch, follow [how-to-build](./how-to-build.md) and [how-to-install](./how-to-install.md). -3. Start Gravitino using the gravitino.sh script: +3. Start Gravitino using the gravitino.sh script in the binary release package or Docker image: ```shell /bin/gravitino.sh start ``` +4. Start Gravitino using the gravitino.sh script in the distribution package: + + ```shell + ./gradlew compileDistribution -x test + /distribution/package/bin/gravitino.sh start + ``` Review Comment: Thank you for the feedback. I will remove these lines from the documentation. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
jerryshao commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1908268193 ## docs/getting-started.md: ## @@ -176,11 +176,17 @@ To use Gravitino locally on macOS or Linux, follow these similar steps: Or, you can install Gravitino from scratch, follow [how-to-build](./how-to-build.md) and [how-to-install](./how-to-install.md). -3. Start Gravitino using the gravitino.sh script: +3. Start Gravitino using the gravitino.sh script in the binary release package or Docker image: ```shell /bin/gravitino.sh start ``` +4. Start Gravitino using the gravitino.sh script in the distribution package: + + ```shell + ./gradlew compileDistribution -x test + /distribution/package/bin/gravitino.sh start + ``` Review Comment: I feel this is unnecessary to add. `getting-started.md` is facing the end users, they already have the binary package or docker image at hand, and they don't need to compile from the ground and run from source code, so it is not proper to add this here. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
jerryshao commented on code in PR #5977:
URL: https://github.com/apache/gravitino/pull/5977#discussion_r1908262960
##
bin/common.sh:
##
@@ -42,6 +42,11 @@ if [[ -f "${GRAVITINO_CONF_DIR}/gravitino-env.sh" ]]; then
. "${GRAVITINO_CONF_DIR}/gravitino-env.sh"
fi
+if [[ -z "${GRAVITINO_VERSION}" ]]; then
+ echo -e "No GRAVITINO_VERSION was found, you may need to:\n1. Run the
gravitino.sh script on the compiled Gravitino.\n2. Run the gravitino.sh script
in the release package."
Review Comment:
This line is too long, we typically limit the length of the line to 100
characters, can you please break into several 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
jerryshao commented on PR #5977: URL: https://github.com/apache/gravitino/pull/5977#issuecomment-2579311862 > > I see you change the script name from `.sh` to `.sh.template` without any gradle script change to rename back to `.sh`, does it really work? > > Yes, it can work because, in the line [601](https://github.com/apache/gravitino/blob/e9d8ee7bc05d3226c5f0ce0b492b2c207018ed73/build.gradle.kts#L601) of the root `build.gradle.kts`, the build task will copy all files under the path `bin` into `distribution/package/bin` and replace .template. Adding .template to .sh files under the `bin` path is intended to prevent developers from being misled by their extensions. I see, thanks for the explanation. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on PR #5977: URL: https://github.com/apache/gravitino/pull/5977#issuecomment-2579303689 > I see you change the script name from `.sh` to `.sh.template` without any gradle script change to rename back to `.sh`, does it really work? Yes, it can work because, in the line [601](https://github.com/apache/gravitino/blob/e9d8ee7bc05d3226c5f0ce0b492b2c207018ed73/build.gradle.kts#L601) of the root `build.gradle.kts`, the build task will copy all files under the path `bin` into `distribution/package/bin` and replace .template. Adding .template to .sh files under the `bin` path is intended to prevent developers from being misled by their extensions. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
jerryshao commented on PR #5977: URL: https://github.com/apache/gravitino/pull/5977#issuecomment-2579259622 I see you change the script name from `.sh` to `.sh.template` without any gradle script change to rename back to `.sh`, does it really work? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
xunliu commented on PR #5977: URL: https://github.com/apache/gravitino/pull/5977#issuecomment-2574376651 hi @liuchunhao Please update `docs/getting-started.md#getting-started-locally` 3. Start Gravitino using the gravitino.sh script in the binary release package or Docker image: ``` /bin/gravitino.sh start ``` 4. Start Gravitino using the gravitino.sh script in the distribution package: ``` ./gradlew compileDistribution -x test /distribution/package/bin/gravitino.sh start ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
justinmclean commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1899217004 ## bin/common.sh: ## @@ -18,6 +18,13 @@ # Referred from Apache Submarine's common.sh implementation # bin/common.sh +GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER +if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then + echo "GRAVITINO_VERSION is not set. Please make sure you are running the script from the distribution/package/bin and before running the script, run './gradle clean build -x test compileDistribution'" + exit 1 Review Comment: Ask them to use a binary distribution or compile it first? i.e: 1. Run the gravitino.sh script in a compiled version of Gravitino. 2. Run the gravitino.sh script from a binary release package. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
xunliu commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1898785837 ## bin/common.sh: ## @@ -18,6 +18,13 @@ # Referred from Apache Submarine's common.sh implementation # bin/common.sh +GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER +if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then + echo "GRAVITINO_VERSION is not set. Please make sure you are running the script from the distribution/package/bin and before running the script, run './gradle clean build -x test compileDistribution'" + exit 1 Review Comment: @justinmclean The problem is that there are some users who run gravitino.sh scripts without compiling, causing them to fail. We want to give them some tips. Do you have any better suggestions? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1897637617 ## bin/common.sh: ## @@ -18,6 +18,13 @@ # Referred from Apache Submarine's common.sh implementation # bin/common.sh +GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER +if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then + echo "GRAVITINO_VERSION is not set. Please make sure you are running the script from the distribution/package/bin and before running the script, run './gradle clean build -x test compileDistribution'" + exit 1 Review Comment: What does 'release package' mean? Should it instead be referred to as a 'distribution package'? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on PR #5977: URL: https://github.com/apache/gravitino/pull/5977#issuecomment-2562021955 > I will use your PR to test, because `gravitino.sh` is very important. So, I need some time to review this PR. Thanks. I ran `./gradle clean build compileDistribution` with all test tasks, and everything seems fine. After launching `./gravitino.sh start`, I sent an API call to /api/version, and it worked fine.  -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
justinmclean commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1897496152 ## bin/common.sh: ## @@ -18,6 +18,13 @@ # Referred from Apache Submarine's common.sh implementation # bin/common.sh +GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER +if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then + echo "GRAVITINO_VERSION is not set. Please make sure you are running the script from the distribution/package/bin and before running the script, run './gradle clean build -x test compileDistribution'" + exit 1 Review Comment: "Run the gravitino.sh script in the release package." this may not be the best instructions, as an ASF release package is source, not binary. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1897061603 ## bin/common.sh: ## @@ -18,6 +18,13 @@ # Referred from Apache Submarine's common.sh implementation # bin/common.sh +GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER +if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then + echo "GRAVITINO_VERSION is not set. Please make sure you are running the script from the distribution/package/bin and before running the script, run './gradle clean build -x test compileDistribution'" + exit 1 Review Comment: Thank you for the feedback. I will make the updates as required: - Update the error message for startup failures - Adjust the version check - Revert the root build.gradle.kts 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
xunliu commented on code in PR #5977: URL: https://github.com/apache/gravitino/pull/5977#discussion_r1897055946 ## bin/common.sh: ## @@ -18,6 +18,13 @@ # Referred from Apache Submarine's common.sh implementation # bin/common.sh +GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER +if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then + echo "GRAVITINO_VERSION is not set. Please make sure you are running the script from the distribution/package/bin and before running the script, run './gradle clean build -x test compileDistribution'" + exit 1 Review Comment: Please update the prompt message: ``` No GRAVITINO_VERSION was found, you may need to: 1. Run the gravitino.sh script on the compiled Gravitino. 2. Run the gravitino.sh script in the release package. ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
xunliu commented on PR #5977: URL: https://github.com/apache/gravitino/pull/5977#issuecomment-2561552440 @liuchunhao Thank you for your improve user experience. Some Gravitino new user do not know the need compile the Gravitino first, but directly execution `bin/gravitino.sh` in the project root path, and find that it cannot be used, causing confusion to new users. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
tengqm commented on PR #5977: URL: https://github.com/apache/gravitino/pull/5977#issuecomment-2561534938 This smells like an over engineering effort although I do appreciate the intention. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
[PR] [#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage [gravitino]
liuchunhao opened a new pull request, #5977:
URL: https://github.com/apache/gravitino/pull/5977
### What changes were proposed in this pull request?
- Add file suffix ‘template’ to the following scripts:
- bin/gravitino.sh
- bin/common.sh
- bin/gravitino-iceberg-rest-server.sh
- Add a validation check on `GRAVITINO_VERSION` in the script bin/common.sh
( renamed to bin/common.sh.template ) with the followings :
```bash
GRAVITINO_VERSION=GRAVITINO_VERSION_PLACEHOLDER
if [[ "$GRAVITINO_VERSION" == *_VERSION_PLACEHOLDER ]]; then
echo "GRAVITINO_VERSION is not set. Please make sure you are running
the script from the distribution/package/bin and before running the script, run
'./gradle clean build -x test compileDistribution'"
exit 1
fi
```
- Update the following tasks in the root build.gradle.kts as described below
:
- compileDistribution
- compileIcebergRESTServer
```bash
eachFile {
if (name == "gravitino-env.sh" || name == "common.sh") {
filter { line ->
line.replace("GRAVITINO_VERSION_PLACEHOLDER", "$version")
}
}
}
```
### Why are the changes needed?
To prevent incorrect usage with startup scripts
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
- The scripts below will exit with status 1 and print an error message with
the correct instructions
```bash
cd bin
gravitino.sh.template start
gravitino-iceberg-rest-server.sh.template start
```
- correct way to run gravitino :
```bash
./gradle clean build -x test compileDistribution
cd distribution/package/bin
./gravitino.sh start
./gravitino-iceberg-rest-server.sh start
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
