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.

Sign up for GitHub

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

Jump to bottom

Issue #10271 - Introduce jetty-pid.xml #10272

Merged
merged 23 commits into from
Aug 25, 2023
Merged

Issue #10271 - Introduce jetty-pid.xml #10272

merged 23 commits into from
Aug 25, 2023

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Aug 8, 2023 •
edited
Loading

New jetty-pid.xml, used by jetty.sh functions similar to jetty-started.xml and will have Jetty itself produce (and remove) a pid file based on configuration in jetty.sh (eg: the $JETTY_PID value)

@joakime
Issue #10271 - new start.jar --pid-file=<name> arg
adca81d 
+ Have jetty.sh use --pid-file=<name> too

Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime joakime added the Bug For general bugs on Jetty side label Aug 8, 2023
@joakime joakime self-assigned this Aug 8, 2023
@joakime joakime marked this pull request as draft August 8, 2023 13:50
@joakime joakime added this to the 10.0.x milestone Aug 8, 2023
@joakime joakime linked an issue Aug 8, 2023 that may be closed by this pull request
jetty.sh does not stop jetty anymore #10271
Closed
@joakime joakime mentioned this pull request Aug 8, 2023
Closed
@joakime
Fixing license header
6133dd0 
Signed-off-by: Joakim Erdfelt <[email protected]>
sbordet
sbordet requested changes Aug 8, 2023
View reviewed changes
joakime added 2 commits August 8, 2023 11:09
@joakime
Changes from review
a0dbc2a 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Changes from review
8e7e10f 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime joakime requested a review from sbordet August 8, 2023 16:28
@joakime joakime changed the title Issue #10271 - new start.jar --pid-file=<name> arg Issue #10271 - Introduce jetty-pid.xml Aug 8, 2023
gregw
gregw requested changes Aug 8, 2023
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

I now think the PID mechanism should be different from the started mechanism, as the PID relates to the process and not the server.

Thus I think it should not be a LifeCycleListener and instead should be it's own shutdown hook. As soon as the hook is created, it should create the pid file and register a shutdown hook that will delete the file only as the JVM stops, which is technically unrelated to any stop/start/fails that the server might do.

@joakime
Copy link
Contributor Author

joakime commented Aug 8, 2023

I now think the PID mechanism should be different from the started mechanism, as the PID relates to the process and not the server.

Thus I think it should not be a LifeCycleListener and instead should be it's own shutdown hook. As soon as the hook is created, it should create the pid file and register a shutdown hook that will delete the file only as the JVM stops, which is technically unrelated to any stop/start/fails that the server might do.

I'll give that a whirl, in a different PR, to see how it looks.

@joakime
Changes from review
778bbb9 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Copy link
Contributor Author

joakime commented Aug 8, 2023

@gregw I reworked this PR (no new PR) to be based on Shutdown behaviors.
WDYT?

@joakime joakime requested a review from gregw August 8, 2023 18:51
gregw
gregw reviewed Aug 8, 2023
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

This looks nice! Just comment at this stage, will review for approval once not a draft.

joakime added 3 commits August 9, 2023 10:27
@joakime
More cleanup
4c91aed 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Ensure pidfile is truncated on write (so only 1 pid is present)
3b8f159 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime
Javadoc
394db41 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime joakime marked this pull request as ready for review August 9, 2023 15:30
@joakime joakime requested review from sbordet and gregw August 14, 2023 18:00
@joakime
Use ShutdownThread, not ShutdownMonitor.
443c9c9 
Moved Pid module & files to jetty-util

Signed-off-by: Joakim Erdfelt <[email protected]>
sbordet
sbordet requested changes Aug 16, 2023
View reviewed changes
@joakime joakime requested a review from sbordet August 21, 2023 16:24
gregw
gregw previously approved these changes Aug 21, 2023
View reviewed changes
lorban
lorban requested changes Aug 22, 2023
View reviewed changes
@joakime
Changes from review
724dbb9 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime joakime requested a review from lorban August 22, 2023 19:39
@joakime
Deregister PidFile on stop
a8f9f6d 
Signed-off-by: Joakim Erdfelt <[email protected]>
@joakime joakime requested a review from gregw August 22, 2023 23:21
@joakime joakime requested a review from gregw August 24, 2023 01:18
gregw
gregw approved these changes Aug 25, 2023
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

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

LGTM

@joakime joakime merged commit 900f50f into jetty-10.0.x Aug 25, 2023
@joakime joakime deleted the fix/10.0.x/start-pidfile branch August 25, 2023 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakim joakim joakim left review comments

@gregw gregw gregw approved these changes

@sbordet sbordet sbordet approved these changes

@lorban lorban lorban approved these changes

@olamy olamy Awaiting requested review from olamy

@lachlan-roberts lachlan-roberts Awaiting requested review from lachlan-roberts

Assignees

@joakime joakime

Labels
Bug For general bugs on Jetty side
Projects
None yet
Milestone
10.0.x
Development

Successfully merging this pull request may close these issues.

jetty.sh does not stop jetty anymore
5 participants
@joakime @joakim @gregw @sbordet @lorban