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

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 <joakim.erdfelt@gmail.com>
@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 <joakim.erdfelt@gmail.com>
sbordet
sbordet requested changes Aug 8, 2023
View reviewed changes
jetty-home/src/main/resources/bin/jetty.sh Outdated Show resolved Hide resolved
jetty-home/src/main/resources/bin/jetty.sh Outdated Show resolved Hide resolved
jetty-home/src/main/resources/bin/jetty.sh Outdated Show resolved Hide resolved
jetty-home/src/main/resources/etc/jetty-pid.xml Outdated Show resolved Hide resolved
jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java Outdated Show resolved Hide resolved
jetty-start/src/main/java/org/eclipse/jetty/start/StartArgs.java Outdated Show resolved Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/component/PidFileLifeCycleListener.java Outdated Show resolved Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/component/PidFileLifeCycleListener.java Outdated Show resolved Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/component/PidFileLifeCycleListener.java Outdated Show resolved Hide resolved
@joakime
Changes from review
a0dbc2a 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Changes from review
8e7e10f 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@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.

jetty-home/src/main/resources/etc/jetty.conf Outdated Show resolved Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/component/PidFileLifeCycleListener.java Outdated Show resolved Hide resolved
@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 <joakim.erdfelt@gmail.com>
@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.

jetty-server/src/main/java/org/eclipse/jetty/server/PidFile.java Outdated Show resolved Hide resolved
jetty-server/src/main/config/modules/pid.mod Outdated Show resolved Hide resolved
jetty-home/src/main/resources/etc/jetty.conf Outdated Show resolved Hide resolved
@joakime
More cleanup
4c91aed 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Ensure pidfile is truncated on write (so only 1 pid is present)
3b8f159 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Javadoc
394db41 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@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 <joakim.erdfelt@gmail.com>
sbordet
sbordet requested changes Aug 16, 2023
View reviewed changes
jetty-server/src/main/config/modules/state.mod Outdated Show resolved Hide resolved
jetty-util/src/main/config/modules/pid.mod Outdated Show resolved Hide resolved
jetty-util/src/main/config/modules/pid.mod Show resolved Hide resolved
@joakime
Commenting out the property in the ini-template
0323557 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from sbordet August 21, 2023 16:24
@joakime
Merge remote-tracking branch 'origin/jetty-10.0.x' into fix/10.0.x/st…
b3368ae 
…art-pidfile
@joakime
Fixing license header
565f55a 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
gregw
gregw previously approved these changes Aug 21, 2023
View reviewed changes
lorban
lorban requested changes Aug 22, 2023
View reviewed changes
jetty-util/src/main/config/etc/jetty-pid.xml Outdated Show resolved Hide resolved
jetty-server/src/main/config/etc/jetty-state.xml Outdated Show resolved Hide resolved
jetty-util/src/main/java/org/eclipse/jetty/util/PidFile.java Outdated Show resolved Hide resolved
@joakime
Changes from review
724dbb9 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Add DEBUG logging to show actual filename used in PidFile and StateLi…
f4258ec 
…feCycleListener

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from lorban August 22, 2023 19:39
@joakime
Deregister PidFile on stop
a8f9f6d 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from gregw August 22, 2023 23:21
@joakime
Simplify PidFile and add distro tests for pid module
453fa9a 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Better PidFile management
e851ca7 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime joakime requested a review from gregw August 24, 2023 01:18
@joakime
state.mod depends on server as it configures the server.
d88d617 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@joakime
Updates from review
33f6418 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
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
4 checks passed
@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