[#150][#178] feat(all): Add launching process and scripts #191

xunliu · 2023-08-08T11:17:50Z

What changes were proposed in this pull request?

Through graviton.sh script launching GravtionServer.

Command

./bin/graviton.sh [--config <conf-dir>] {start|stop|restart|status}

Directory struct

├── bin
│   ├── command.sh
│   └── graviton.sh
├── conf
│   ├── graviton.conf.template
│   ├── graviton-env.sh.template
│   └── log4j.properties.template
└── distribution/package
    ├── bin/
    ├── catalogs/catalog-hive
    ├── conf/
    ├── lib/
    ├── graviton-${version}-bin.tar
    └── graviton-${version}-bin.tar.sha256

Scripts context

bin/command.sh: Command functions
bin/graviton.sh: Launching scripts
conf/graviton.conf.template: All configuration templates for Gravtion
conf/gravtion-env.sh.template: Environment variables, etc., JAVA_HOME, GRAVITON_HOME etc.,
conf/log4j.properties.template: log4j configuration for Gravtion Server.

Assemble Distribution

./gradlew clean build
./gradlew compileDistribution
- Copy ${project_root_dir}/bin/ directory to ${project_root_dir}/distribution/package/bin/.
- Copy ${project_root_dir}/conf/ directory to ${project_root_dir}/distribution/package/conf/ and remove .template file name suffix.
- Copy ${project_root_dir}/${sub_module}/lib to ${project_root_dir}/distribution/package/lib/
- Copy ${project_root_dir}/catalog-hive}/lib to ${project_root_dir}/distribution/package/catalogs/catalog-hive/lib
./gradlew assembleDistribution
- Compress ${project_root_dir}/distribution/package generate graviton-${version}-bin.tar and graviton-${version}-bin.tar.sha256
./distribution/package/bin/graviton.sh start|stop|status

Why are the changes needed?

Through script launching Graviton Server.

Fix: #178

Does this PR introduce any user-facing change?

N/A

How was this patch tested?

Test on PullRequst submission

github-actions · 2023-08-08T11:19:29Z

Code Coverage Report

Overall Project	59.9% `-0.26%`	🟢
Files changed	43.31%	🔴

Module	Coverage
server	81.56% `-2.27%`	🔴

Files

Module	File	Coverage
server	ServerConfig.java	100%	🟢
	JettyServer.java	86.96%	🟢
	GravitonServer.java	64.14% `-24.24%`	🔴
	ProjectVersion.java	0%	🔴

yuqi1129 · 2023-08-08T11:57:07Z

.github/workflows/build.yml

+        run: |
+          gradle assembleDistribution
+          ls -l distribution/package


What's the usage of the ls command?

print package directory to help check.

yuqi1129 · 2023-08-08T11:59:55Z

server/src/main/java/com/datastrato/graviton/server/web/JettyServer.java


    // Create and config Jetty Server
-    server = new Server(threadPool);
+    server = new Server(threadPool2);


I may not get the context, Why we need to use a new thread pool?

yuqi1129 · 2023-08-08T12:01:17Z

server/src/main/java/com/datastrato/graviton/server/GravitonServer.java

+                  } catch (Exception e) {
+                    LOG.error("Error while stopping servlet container", e);
+                  }
+                  LOG.info("Done, Shutting down Graviton Server.");


Shutting down Graviton Server.

Better: Graviton Server has shut down

yuqi1129 · 2023-08-08T12:06:06Z

core/src/main/java/com/datastrato/graviton/Config.java

+      for (String key : properties.stringPropertyNames()) {
+        String value = properties.getProperty(key);
+        String[] splits = value.split("# ");
+        if (splits.length == 2) {


I am afraid the condition that length of array is 2 is not enough, see:

configkey configvalue # # this is comment

This code section has been removed.

xunliu · 2023-08-08T14:02:42Z

@jerryshao Please help me to review this PR. thanks.

jerryshao · 2023-08-09T11:47:52Z

.github/workflows/build.yml

+            echo "Failed to start graviton"
+            exit 1
+          fi


I don't think it is super necessary to start and test a server here, this will prolong the CI time.

Removed bin/graviont.sh start test. And split Integration Test into a single workflow.

jerryshao · 2023-08-09T11:50:11Z

.github/workflows/build.yml

+      - name: Package with Gradle
+        run: |
+          gradle assembleDistribution


I don't think it is necessary to add package process here in build.yml, this script is mainly for UT. Is it better to use another workflow for package things?

Remove the compress tar.gz process, only retain only create distribution/package directory.

Split Integration Test into a single workflow.

jerryshao · 2023-08-09T12:03:07Z

bin/common.sh

+    fi
+
+    if [ "$JVM_VERSION" -lt 8 ] || { [ "$JVM_VERSION" -eq 8 ] && [ "${jvmver#*_}" -lt 151 ]; } ; then
+        echo "Graviton requires either Java 8 update 151 or newer"


Is Java 8 enough, do we require a Java version larger than 151? Also, I was worrying about the way you get Java version using regex, you may not get the version number from some customized java release like zuul, kona jdk?

Because after JDK 8u151, Oracle fixed a number of security vulnerabilities and issues to improve system stability and security. We suggest using after JDK 8u151.
https://www.oracle.com/java/technologies/javase/8u151-relnotes.html

Because we only test passed in Oracle JDK, Other JDK versions not tested, better limit JDK type & version.

I would suggest not using Oracle JDK 8 as it has a restrictive license. Oracle JDK Java 8 is under the OTN License Agreement for Java (https://www.oracle.com/cis/downloads/licenses/oracle-javase-license.html). This requires a fee / license for commercial use.

I would suggest using OpenJDK / Corretto.

hi, @justinmclean thank you for your suggestion.
I create issue #215 to track this problem.

jerryshao · 2023-08-09T12:22:56Z

bin/graviton.sh

+if [[ "$1" == "--config" ]]; then
+  shift
+  conf_dir="$1"
+  if [[ ! -d "${conf_dir}" ]]; then


May be it is not necessary to add --config <conf-dir>? I think we can use GRAVITON_CONF_DIR or GRAVITON_HOME to infer the conf dir programatically.

The --config parameter is an option that allows us to easily change the configuration directory in the start scripts.

When the --config parameter is not specified, GRAVITON_CONF_DIR is the default environment in the graviton-env.sh

When the --config parameter is specified, the GRAVITON_CONF_DIR environment is overwritten.

jerryshao · 2023-08-09T12:25:55Z

bin/graviton.sh

+
+function found_graviton_server_pid() {
+  process_name='GravitonServer';
+  RUNNING_PIDS=$(ps x | grep ${process_name} | grep -v grep | awk '{print $1}');


Is it better to write pid to a file to avoid same name process?

Using PID file is a traditional operation, and I have implemented this function in another version.

Generation the PID file in the graviton.sh start command.

Deleted the PID file in the graviton.sh stop command.

However, when the Graviton Server itself exits abnormally, the PID file is not deleted properly.
You will not be able to restart Gravtion Server the next time because there is already an old PID file.
So I think might be better to get the directory PID in the operation system.

I suggest that we can keep this implementation, and consider modifying it later when we have a specific problem.
What do you think?

jerryshao · 2023-08-09T14:10:42Z

build.gradle.kts

+      if (it.name == "catalog-hive") {
+        // println("copyCatalogRuntimeClass: ${it.name}")
+        from(it.configurations.runtimeClasspath)
+        into("distribution/package/catalog/catalog-hive/lib")


catalogs/catalog-hive

jerryshao · 2023-08-09T14:11:31Z

build.gradle.kts

+      if (it.name != "catalog-hive") {
+        // println("copyRuntimeClass: ${it.name}")
+        from(it.configurations.runtimeClasspath)
+        into("distribution/package/lib")


we don't need to copy client jar into lib?

The catalog-hive JAR need copy to another directory(distribution/package/catalog/catalog-hive/lib), So I split two different Tasks, copyCatalogRuntimeClass() and copySubmoduleClass().

What I mean is client-jar lib, I think we don't need to include this module's jar files into server's lib.

jerryshao · 2023-08-09T14:13:04Z

core/src/main/java/com/datastrato/graviton/Config.java

@@ -133,7 +133,6 @@ Properties loadPropertiesFromFile(File file) throws IOException {
    try (InputStream in = Files.newInputStream(file.toPath())) {
      properties.load(in);
      return properties;
-


this seems like a unnecessary change?

This change has been rollback.

jerryshao · 2023-08-09T14:14:18Z

server/src/main/java/com/datastrato/graviton/server/GravitonServer.java

+                  LOG.info("Shutting down Graviton Server ... ");
+                  try {
+                    server.stop();
+                    Thread.sleep(3000);


we'd better not make this hardcoded.

OK, added SERVER_SHUTDOWN_TIMEOUT into Configure.

jerryshao · 2023-08-09T14:15:57Z

server/src/main/resources/log4j2.properties

+rootLogger.level = debug
+
+# Root logger referring to console appender
+rootLogger.appenderRef.stdout.ref = consoleLogger


this file seems not necessary, as we already removed this in the previous pr.

Removed this log4j2.properties file.

jerryshao · 2023-08-10T08:09:48Z

bin/common.sh

+function addEachJarInDirRecursive(){
+  if [[ -d "${1}" ]]; then
+    for jar in "${1}"/**/*.jar ; do
+      GRAVITON_CLASSPATH="$jar:$GRAVITON_CLASSPATH"


I would suggest to add "{}" to variables to avoid potentially issues.

jerryshao · 2023-08-10T08:19:46Z

build.gradle.kts

+        println()
+      }
+    }
+  }


Seems we only print some dependency information here for this allDeps task, is this necessary? Also gradle has a similar functionality.

I using a new function to replace it.

val allDeps by tasks.registering(DependencyReportTask::class)

Now if you execute ./gradlew allDeps command, will print all subproject dependency

jerryshao · 2023-08-10T08:22:58Z

server/src/main/java/com/datastrato/graviton/server/GravitonServer.java

+                  LOG.info("Shutting down Graviton Server ... ");
+                  try {
+                    server.stop();
+                    Thread.sleep(server.serverConfig.get(ServerConfig.SERVER_SHUTDOWN_TIMEOUT));


Do we also need to put this configuration into graviton.conf.template?

Added it into graviton.conf.template.

jerryshao · 2023-08-10T08:24:42Z

server/src/main/java/com/datastrato/graviton/server/web/JettyServer.java

@@ -185,7 +188,7 @@ private ExecutorThreadPool createThreadPool(int coreThreads, int maxThreads) {
            maxThreads,
            60,
            TimeUnit.SECONDS,
-            new LinkedBlockingQueue<>(),
+            new LinkedBlockingQueue<>(100),


Do we need to make this configurable?

@jerryshao I think so.
An unbounded queue is dangerous and not recommended in Java when using thread pool. here I notice this problem and advice @xunliu to change here.

Added graviton.server.webserver.executorThreadPoolSize configuration parameter to graviton.conf.

jerryshao · 2023-08-10T08:32:06Z

bin/common.sh

+        echo "Graviton requires either Java 8 update 151 or newer"
+        exit 1;
+    fi
+}


The indention of this method is 4 spaces.

The indention of this method is 4 spaces.

This is not fixed.

jerryshao · 2023-08-10T11:07:49Z

server/src/main/java/com/datastrato/graviton/server/ServerConfig.java

@@ -59,6 +59,13 @@ public class ServerConfig extends Config {
          .intConf()
          .createWithDefault(128 * 1024);

+  public static final ConfigEntry<Integer> WEBSERVER_EXECUTOR_THREAD_POOL_SIZE =
+      new ConfigBuilder("graviton.server.webserver.executorThreadPoolSize")


This configuration may not be the thread pool size? I think it should be queue size?

OK, modify to WEBSERVER_THREAD_POOL_WORK_QUEUE_SIZE.

jerryshao

LGTM. I'm going to merge this PR. We don't restrict the JDK vendor version, I think there's no issue here, and can be addressed later if necessary.

yuqi1129 reviewed Aug 8, 2023

View reviewed changes

apache deleted a comment from yuqi1129 Aug 8, 2023

xunliu requested a review from jerryshao August 8, 2023 14:02

jerryshao reviewed Aug 9, 2023

View reviewed changes

xunliu added 22 commits August 10, 2023 09:50

Add launching process and scripts

b0ca400

execute graviton.sh success

583b25c

temp

59ffc93

graviton start|stop|status

10e5e9a

add github action

be53e4b

add dependsOn

e715478

add gravition.sh restart

cc2632f

update graviton.sh

b0befed

rollback

d6e6032

Fix jetty configure error

f6e56ab

Add project version restful API

d569452

fixed

9840fa8

enable script log

026552a

disable graviton.sh log

53e1e45

Use tar compress

5248c59

gradle compileDistribution

1f0f2c8

fix

c1ddbb3

add integration workflow in the Github

c69d702

Modify Github workflow job name

79c756c

exclude client-java JAR from server/lib

de2fe6e

fix

039b66f

Remove project.properties file when gradle clean command execute

4fb0c58

xunliu closed this Aug 10, 2023

xunliu reopened this Aug 10, 2023

jerryshao reviewed Aug 10, 2023

View reviewed changes

xunliu changed the title ~~[#178] feat(all): Add launching process and scripts~~ [#150][#178] feat(all): Add launching process and scripts Aug 10, 2023

Fix some problem

2fd559b

jerryshao reviewed Aug 10, 2023

View reviewed changes

xunliu added 4 commits August 10, 2023 19:18

Modify WEBSERVER_THREAD_POOL_WORK_QUEUE_SIZE

5b82b46

removed < /dev/null from graviton.sh

b62ffda

space indent

75346fd

Add comment about JDK 8u151

fb45d5a

jerryshao approved these changes Aug 11, 2023

View reviewed changes

jerryshao merged commit b97e382 into apache:main Aug 11, 2023
2 checks passed

xunliu deleted the issue-178 branch August 28, 2023 05:59

[#150][#178] feat(all): Add launching process and scripts #191

[#150][#178] feat(all): Add launching process and scripts #191

Conversation

xunliu commented Aug 8, 2023 • edited Loading

What changes were proposed in this pull request?

Command

Directory struct

Scripts context

Assemble Distribution

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

github-actions bot commented Aug 8, 2023 • edited Loading

Code Coverage Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinmclean Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryshao left a comment

Choose a reason for hiding this comment

xunliu commented Aug 8, 2023 •

edited

Loading

github-actions bot commented Aug 8, 2023 •

edited

Loading

justinmclean Aug 11, 2023 •

edited

Loading

xunliu Aug 10, 2023 •

edited

Loading