Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#5976] Improvement(bin):Add validation checks to the startup scripts to prevent incorrect usage #5977

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

liuchunhao
Copy link
Contributor

@liuchunhao liuchunhao commented Dec 24, 2024

What changes were proposed in this pull request?

#5976

  • 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 :

    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
     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
cd bin
gravitino.sh.template start    
gravitino-iceberg-rest-server.sh.template start
  • correct way to run gravitino :
./gradle clean build -x test compileDistribution

cd distribution/package/bin

./gravitino.sh start

./gravitino-iceberg-rest-server.sh start

@tengqm
Copy link
Contributor

tengqm commented Dec 25, 2024

This smells like an over engineering effort although I do appreciate the intention.

@xunliu
Copy link
Member

xunliu commented Dec 25, 2024

@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.

bin/common.sh Outdated
Comment on lines 23 to 24
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

…failures /Adjust the version check /Revert the root build.gradle.kts files
Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will use your PR to test, because gravitino.sh is very important.
So, I need some time to review this PR. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants