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

Backport hz start to 5.0.z #20812

Merged
merged 5 commits into from
Mar 3, 2022

Conversation

frant-hartm
Copy link
Contributor

@frant-hartm frant-hartm commented Feb 22, 2022

We want to backport the hz start script to provide a unified experience across recent versions.

This is a backport of the following PRs:

#20262
#20346
#20489
#19749 (small improvement in error message, makes the following PR easier to backport)
#20687

The backport is clean apart from the #20687 which removes code changed on the main branch but not in 5.0.z.

Checklist:

  • Labels (Team:, Type:, Source:, Module:) and Milestone set
  • Label Add to Release Notes or Not Release Notes content set
  • Request reviewers if possible
  • Send backports/forwardports if fix needs to be applied to past/future releases

ldziedziul and others added 5 commits February 22, 2022 15:43
…20262)

* Add hz to distribution package HZ-625

* Remove not used classpath elements from hz script HZ-625

* Usage HAZELCAST_CONFIG in hz HZ-625

* Use prometheus version from variable HZ-625

* Checkstyle fix HZ-625

* Remove mc subcommand HZ-625

* Use BuildInfoProvider for getting hz version HZ-625

* Remove not used resources HZ-625

* Use hz-stop to stop instances started by hz command HZ-625

* Add option to hz command to run Hazelcast instance as daemon HZ-625

* Delegate "hz-start" to "hz start" HZ-625

* Update distribution/src/test/java/com/hazelcast/distribution/DistributionIT.java

Co-authored-by: František Hartman <frant.hartm@gmail.com>

* Inline AbstractCommandLine

* Suppress output in daemon mode

* Remove redundant config loader

* Use standard system properties for setting port and interface in hz command

* Rename HazelcastCommandLine to HazelcastServerCommandLine

* Use HazelcastMemberStarter to start the instance

* Use nohup for daemon mode in hz command

* Simplify finding script location in hz

* Simplify CLASSPATH in hz

* Print always java details when running hz

* Revert "Use hz-stop to stop instances started by hz command HZ-625"

This reverts commit ff6daa6.

* Replace workingdirectory property with HAZELCAST_HOME env

* Remove Runnable interface from HazelcastServerCommandLine

* Remove unused resource file

* Cleanup logging in hz command
The Hazelcast process wasn't being killed correctly and in combination
with java.lang.ProcessBuilder#inheritIO the JVM running the test was
hanging at shut down (the test passed correctly).

There were 2 issues causing improper shutdown:

 - the scripts hz/hz-start were not using exec command which replaces
current shell process with the command, this resulted in killing the
shell, but that doesn't immediatelly kill the HazelcastServerCommandLine
process.  Using exec means we kill the HazelcastServerCommandLine
directly.

 - similarly the HazelcastServerCommandLine starts a new process
(HazelcastMemberStarter) and when it is killed it doesn't automatically
kill the new process. Added shutdown hook so the process is killed in
normal scenario (it won't be killed correclty if `kill -9` is used, but
we can't do anything about that).

Fixes hazelcast/hazelcast-enterprise#4425
Intermediate Java process was used to provide additional classpath or
JVM parameters from command line parameters.

However, there are issues with the approach:
- there are 2 JVMs running, consuming resources
- it's not clear to the user what the processes are for, what should be
killed
- killing with `kill -9` the parent process actually doesn't kill the
child process
@frant-hartm
Copy link
Contributor Author

@vbekiaris I just want to double-check that it is ok to backport this, as it's kind of a new feature and not trivial (but neither really complex).

Copy link
Contributor

@ldziedziul ldziedziul left a comment

Choose a reason for hiding this comment

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

Looks Good-ish ;)

Copy link
Contributor

@ufukyilmaz ufukyilmaz left a comment

Choose a reason for hiding this comment

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

This backport commits look good to me

@frant-hartm frant-hartm merged commit 887f801 into hazelcast:5.0.z Mar 3, 2022
@frant-hartm frant-hartm deleted the backport/hz-server-starter branch March 3, 2022 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants