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

Use exec in wrapper scripts for launching java #100

Merged
merged 2 commits into from Jan 8, 2016

Conversation

Projects
None yet
2 participants
@leedm777
Contributor

leedm777 commented Nov 23, 2015

When launching Java from a wrapper script, it's usually better to use
exec, so that the extra bash process isn't hanging around. It also helps
shutdown and process monitoring (like with upstart, systemd, docker,
etc.) since they would be monitoring (and killing) the Java process
itself, instead of the wrapper script.

Use exec in wrapper scripts for launching java
When launching Java from a wrapper script, it's usually better to use
exec, so that the extra bash process isn't hanging around. It also helps
shutdown and process monitoring (like with upstart, systemd, docker,
etc.) since they would be monitoring (and killing) the Java process
itself, instead of the wrapper script.
@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Nov 24, 2015

Member

I think that's a nice fix, but we need to make sure that it doesn't brake anything. This script will need an update. @damencho WDYT?

@leedm777 Before we merge any of your contributions we need you to sign our contributor agreement (individual or corporate). Apologies if you've already done this. And thank you for the contributions!

Member

bgrozev commented Nov 24, 2015

I think that's a nice fix, but we need to make sure that it doesn't brake anything. This script will need an update. @damencho WDYT?

@leedm777 Before we merge any of your contributions we need you to sign our contributor agreement (individual or corporate). Apologies if you've already done this. And thank you for the contributions!

@leedm777

This comment has been minimized.

Show comment
Hide comment
@leedm777

leedm777 Nov 24, 2015

Contributor

@bgrozev I'm not sure what needs to change in collect-dump-logs.sh. Can you be more specific?

I think my company (Digium) already has a corporate CLA on file. Can you check to see if that would cover my contribution?

Contributor

leedm777 commented Nov 24, 2015

@bgrozev I'm not sure what needs to change in collect-dump-logs.sh. Can you be more specific?

I think my company (Digium) already has a corporate CLA on file. Can you check to see if that would cover my contribution?

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Nov 24, 2015

Member

You do need to sign the CLA again (as the currently signed one doesn't include your name).

For the script, we need to remove PROC_PID and just use PID instead. @damencho just pointed out that we will need to do the same in the init script

Member

bgrozev commented Nov 24, 2015

You do need to sign the CLA again (as the currently signed one doesn't include your name).

For the script, we need to remove PROC_PID and just use PID instead. @damencho just pointed out that we will need to do the same in the init script

@leedm777

This comment has been minimized.

Show comment
Hide comment
@leedm777

leedm777 Nov 24, 2015

Contributor

@bgrozev I've fixed the init and collect dump logs scripts.

I'm a little unsure about the changes to the init script; especially the status option. Do you have a setup you can test that on?

Contributor

leedm777 commented Nov 24, 2015

@bgrozev I've fixed the init and collect dump logs scripts.

I'm a little unsure about the changes to the init script; especially the status option. Do you have a setup you can test that on?

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Nov 30, 2015

Member

@leedm777 Thank you! We'll get this tested when we get a chance.

Member

bgrozev commented Nov 30, 2015

@leedm777 Thank you! We'll get this tested when we get a chance.

@leedm777

This comment has been minimized.

Show comment
Hide comment
@leedm777

leedm777 Dec 1, 2015

Contributor

BTW, I've submitted the signed CLA.

Contributor

leedm777 commented Dec 1, 2015

BTW, I've submitted the signed CLA.

@leedm777

This comment has been minimized.

Show comment
Hide comment
@leedm777

leedm777 Jan 6, 2016

Contributor

bump.

Contributor

leedm777 commented Jan 6, 2016

bump.

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jan 6, 2016

Member

Will test and merge this or next week. Thanks!

Member

bgrozev commented Jan 6, 2016

Will test and merge this or next week. Thanks!

bgrozev added a commit that referenced this pull request Jan 8, 2016

Merge pull request #100 from leedm777/exec-scripts
Use exec in wrapper scripts for launching java

@bgrozev bgrozev merged commit 6b74878 into jitsi:master Jan 8, 2016

@bgrozev

This comment has been minimized.

Show comment
Hide comment
@bgrozev

bgrozev Jan 8, 2016

Member

Merged. Also applied the same to jicofo: jitsi/jicofo@57563e8

Member

bgrozev commented Jan 8, 2016

Merged. Also applied the same to jicofo: jitsi/jicofo@57563e8

@leedm777

This comment has been minimized.

Show comment
Hide comment
@leedm777

leedm777 Jan 11, 2016

Contributor

@bgrozev Very cool. Thanks!

Contributor

leedm777 commented Jan 11, 2016

@bgrozev Very cool. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment