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

Removed PID management from start/stop scripts #15934

Merged
merged 3 commits into from Nov 8, 2019

Conversation

@alparslanavci
Copy link
Contributor

alparslanavci commented Nov 5, 2019

Stop scripts now kill all instances running in the box.

Fixes:
#15831
#15509
#11625

… instances running in the box.
@alparslanavci alparslanavci requested a review from mesutcelik Nov 5, 2019
@mesutcelik

This comment has been minimized.

Copy link
Contributor

mesutcelik commented Nov 5, 2019

@jerrinot I added you as a reviewer. Feel free to review yourself or delegate to someone. This needs at least 2 reviewers.

@mesutcelik mesutcelik requested a review from jerrinot Nov 5, 2019
fi
#### This script kills all the instances started with start.sh script.

PIDS=$(ps ax | grep com.hazelcast.core.server.HazelcastMemberStarter | grep -v grep | awk '{print $1}')

This comment has been minimized.

Copy link
@mesutcelik

mesutcelik Nov 5, 2019

Contributor

It would be good to print pids at least to show that we are killing some processes.

This comment has been minimized.

Copy link
@alparslanavci

alparslanavci Nov 6, 2019

Author Contributor

Added 👍No need to add on Win script, since taskkill already prints some info.

@mesutcelik

This comment has been minimized.

Copy link
Contributor

mesutcelik commented Nov 5, 2019

@alparslanavci Do we have to change anything in the reference manual, code samples etc.?

@alparslanavci

This comment has been minimized.

Copy link
Contributor Author

alparslanavci commented Nov 5, 2019

@mesutcelik yes, we need to change the related reference manual section and the readme file in Enterprise repo.

@mmedenjak

This comment has been minimized.

Copy link
Contributor

mmedenjak commented Nov 6, 2019

Does this address #15509?

@alparslanavci

This comment has been minimized.

Copy link
Contributor Author

alparslanavci commented Nov 6, 2019

@mmedenjak Yes, this also fixes #15509

@hazelcast hazelcast deleted a comment from alparslanavci Nov 6, 2019
@hazelcast hazelcast deleted a comment from alparslanavci Nov 6, 2019
@hazelcast hazelcast deleted a comment from alparslanavci Nov 6, 2019
@hazelcast hazelcast deleted a comment from alparslanavci Nov 6, 2019
@alparslanavci alparslanavci requested review from kwart and removed request for jerrinot Nov 7, 2019
Copy link
Contributor

kwart left a comment

I have some smaller comments. I would also suggest other minor touches in the start.sh file:

  • Use $(...) notation instead of legacy backticked `...`.
  • Double quote paths - e.g. if [ "$JAVA_HOME" ]
  • which is non-standard. Use built-in command -v instead.
echo "Another Hazelcast instance (PID=${PID}) is already started in this folder. To start a new instance, please unzip hazelcast-${project.version}.zip/tar.gz in a new folder."
exit 0
fi
$RUN_JAVA -server $JAVA_OPTS com.hazelcast.core.server.HazelcastMemberStarter &

This comment has been minimized.

Copy link
@kwart

kwart Nov 7, 2019

Contributor

Could we keep it running in the foreground (i.e. remove the trailing &)? So we would fix also the #11625

This comment has been minimized.

Copy link
@alparslanavci

alparslanavci Nov 7, 2019

Author Contributor

👍

@@ -125,8 +125,8 @@ <h3 id="folders">Folders</h3>
<li><strong>hazelcast-full-example.xml</strong> &mdash; It is the Hazelcast IMDG configuration file that includes all the configuration elements and attributes with their descriptions.</li>
<li><strong>start.bat</strong> &mdash; Starts a Hazelcast IMDG member for Windows users.</li>
<li><strong>start.sh</strong> &mdash; Starts a Hazelcast IMDG member for Linux users.</li>
<li><strong>stop.bat</strong> &mdash; Stops the current running Hazelcast IMDG member for Windows users.</li>
<li><strong>stop.sh</strong> &mdash; Stops the current running Hazelcast IMDG member for Linux users.</li>
<li><strong>stop.bat</strong> &mdash; Stops all of the running Hazelcast IMDG member for Windows users.</li>

This comment has been minimized.

Copy link
@kwart

kwart Nov 7, 2019

Contributor

Could we rename the stop scripts to stop-all?

This comment has been minimized.

Copy link
@alparslanavci

alparslanavci Nov 7, 2019

Author Contributor

👍

fi
#### This script kills all the instances started with start.sh script.

PIDS=$(ps ax | grep com.hazelcast.core.server.HazelcastMemberStarter | grep -v grep | awk '{print $1}')

This comment has been minimized.

Copy link
@kwart

kwart Nov 7, 2019

Contributor

Could you consider this:

PIDS=$(pgrep -f HazelcastMemberStarter)

This comment has been minimized.

Copy link
@alparslanavci

alparslanavci Nov 7, 2019

Author Contributor

👍

@kwart

This comment has been minimized.

Copy link
Contributor

kwart commented Nov 7, 2019

We could also give a try to a java.exe on the %Path% in the start.bat:

if "x%JAVA_HOME%" == "x" (
  echo The JAVA_HOME environment variable is not defined! Expecting java on the system PATH.
  set RUN_JAVA=java
) else (
  set "RUN_JAVA=%JAVA_HOME%\bin\java"
)
ECHO ########################################
ECHO # RUN_JAVA=%RUN_JAVA%
ECHO # JAVA_OPTS=%JAVA_OPTS%
ECHO # starting now...."
ECHO ########################################

start "hazelcast %CLASSPATH%" "%RUN_JAVA%" %JAVA_OPTS% -cp "%CLASSPATH%" "com.hazelcast.core.server.HazelcastMemberStarter"
start "hazelcast-imdg" "%RUN_JAVA%" %JAVA_OPTS% -cp "%CLASSPATH%" "com.hazelcast.core.server.HazelcastMemberStarter"

This comment has been minimized.

Copy link
@kwart

kwart Nov 7, 2019

Contributor

I would suggest the same as for the shell script - run it directly in the foreground:

"%RUN_JAVA%" %JAVA_OPTS% -cp "%CLASSPATH%" "com.hazelcast.core.server.HazelcastMemberStarter"

This comment has been minimized.

Copy link
@alparslanavci

alparslanavci Nov 7, 2019

Author Contributor

I kept this one the same, since it is already starting on a separate window.

@alparslanavci

This comment has been minimized.

Copy link
Contributor Author

alparslanavci commented Nov 7, 2019

@kwart thank you for the comments, I applied the most of them. Can you please review again?

@alparslanavci alparslanavci requested a review from kwart Nov 7, 2019
@kwart
kwart approved these changes Nov 7, 2019
Copy link
Contributor

kwart left a comment

LGTM. Thanks for the fixes.

@alparslanavci alparslanavci merged commit 7fb6756 into hazelcast:master Nov 8, 2019
1 check passed
1 check passed
default Test PASSed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.