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 #6476 - warn on exec #6597

Merged
merged 4 commits into from Aug 13, 2021
Merged

Issue #6476 - warn on exec #6597

merged 4 commits into from Aug 13, 2021

Conversation

sbordet
Copy link
Contributor

@sbordet sbordet commented Aug 10, 2021

No description provided.

@sbordet
Improved documentation by correctly redacting out jetty-halt.xml,
af11ff4 
an XML file that is only necessary for rendering the documentation.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Issue #6476 - Show message if JVM args are present but new JVM is spa…
1641053 
…wned

* Added WARN message.
* Updated documentation.
* Added test case.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from gregw August 10, 2021 20:56
@sbordet sbordet linked an issue Aug 10, 2021 that may be closed by this pull request
Show message if JVM args are present but new JVM is spawned based on active modules #6476
Closed
@sbordet sbordet added this to In progress in Jetty 10.0.7/11.0.7 FROZEN via automation Aug 10, 2021
@sbordet
Copy link
Contributor Author

sbordet commented Aug 10, 2021

@gregw see if you want to modify jetty.sh.

@joakime
Copy link
Contributor

joakime commented Aug 10, 2021

The output from --list-config should also indicate if a JVM will be forked or not.

@sbordet
Copy link
Contributor Author

sbordet commented Aug 10, 2021

The output from --list-config should also indicate if a JVM will be forked or not.

You mean that it should be added, or that it is already indicated and should be documented?

@joakime
Copy link
Contributor

joakime commented Aug 10, 2021

The output from --list-config should also indicate if a JVM will be forked or not.

You mean that it should be added, or that it is already indicated and should be documented?

I mean that it should be added (if it isn't there already).

gregw added a commit that referenced this pull request Aug 11, 2021
@gregw
Fix #6597 Use dry run in jetty.sh
96355fd 
Use a --dry-run in jetty.sh to pre-expand the java arguments and thus avoid having two JVMs running in the case of exec.

Also made a small change to allow script to check the current directory for JETTY_BASE, as that allows testing and runs in the same style as direct calls to start.jar

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw mentioned this pull request Aug 11, 2021
Merged
gregw
gregw requested changes Aug 11, 2021
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.

Doco is good. But 3 lines of warning is too much.

jetty-start/src/main/java/org/eclipse/jetty/start/Main.java Outdated
Comment on lines 478 to 480
StartLog.warn("Forking second JVM due to forking module(s): %s.", execModules);
StartLog.warn("Consider using option --dry-run to generate the command line:");
StartLog.warn("$(java -jar $JETTY_HOME/start.jar --dry-run)");
Copy link
Contributor

Choose a reason for hiding this comment

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

3 lines of warning is a bit extreme!

Suggested change
StartLog.warn("Forking second JVM due to forking module(s): %s.", execModules);
StartLog.warn("Consider using option --dry-run to generate the command line:");
StartLog.warn("$(java -jar $JETTY_HOME/start.jar --dry-run)");
StartLog.warn("Forking JVM due to exec in module(s): %s. Use --dry-run to generate non forking command line.", execModules);

Jetty 10.0.7/11.0.7 FROZEN automation moved this from In progress to Review in progress Aug 11, 2021
@sbordet
Issue #6476 - Show message if JVM args are present but new JVM is spa…
ef05630 
…wned

* Updated --list-config to report whether a JVM is forked.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet requested a review from joakime August 11, 2021 08:27
@sbordet
Issue #6476 - Show message if JVM args are present but new JVM is spa…
054b7c7 
…wned

Updates after review: reduced the WARN message lines.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@joakime
Copy link
Contributor

joakime commented Aug 11, 2021

Doco is good. But 3 lines of warning is too much.

I completely disagree with this statement about the number of lines of warning.

This warning is important, it is more important than a normal warning, but not suitable for the next level of severity (eg: error or fatal).

There's TONS of examples of projects with multiline important warnings for a single topic/concept. (docker, k8s, osgi, testcontainers, dropwizard, spring, apt, log4j startup warnings, slf4j init warnings, logback init warnings, node, groovy, maven, etc) I'll be happy to demonstrate each of them if you want.

The reason multiline warnings exists, is that single line warnings is on the wrong side of the "is this important or just noise" question that people have.
We want to cross that threshold with this PR and make sure we report this important information.

joakime
joakime previously requested changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

We need a StartLog.important() level of information conveyance to the user.

@joakime joakime changed the title Jetty 10.0.x 6476 warn on exec Issue #6476 - warn on exec Aug 11, 2021
@gregw
Copy link
Contributor

gregw commented Aug 11, 2021

We have certainly had complaints previously about over verbose logging and there is nothing said in the 3 lines that can't be said in 1.

Asking users to ignore multiple warnings is not good.

Better have an INFO level

gregw added a commit that referenced this pull request Aug 11, 2021
@gregw
Fix #6597 Use dry run in jetty.sh
4cced35 
update from review

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this pull request Aug 11, 2021
@gregw
Fix #6597 Use dry run in jetty.sh
b491e65 
update from review

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@sbordet
Copy link
Contributor Author

sbordet commented Aug 12, 2021

@gregw @joakime I have updated the PR with your suggestions, nudge?

gregw added a commit that referenced this pull request Aug 13, 2021
@gregw
Fix #6597 Use dry run in jetty.sh
d0a2d4c 
update from review

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this pull request Aug 13, 2021
@gregw
Fix #6597 Use dry run in jetty.sh
d998bd1 
update from review

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@joakime joakime dismissed their stale review August 13, 2021 14:53

Consensus was met

Jetty 10.0.7/11.0.7 FROZEN automation moved this from Review in progress to Reviewer approved Aug 13, 2021
@sbordet sbordet merged commit 4af93b5 into jetty-10.0.x Aug 13, 2021
Jetty 10.0.7/11.0.7 FROZEN automation moved this from Reviewer approved to Done Aug 13, 2021
@sbordet sbordet deleted the jetty-10.0.x-6476-warn-on-exec branch August 13, 2021 15:32
sbordet added a commit that referenced this pull request Aug 13, 2021
@gregw @sbordet
Fix #6476 Use dry run in jetty.sh (#6598)
2b0161e 
* Fix #6597 Use dry run in jetty.sh

Use a --dry-run in jetty.sh to pre-expand the java arguments and thus avoid having two JVMs running in the case of exec.

Also made a small change to allow script to check the current directory for JETTY_BASE, as that allows testing and runs in the same style as direct calls to start.jar

Signed-off-by: Greg Wilkins <gregw@webtide.com>
Co-authored-by: Simone Bordet <simone.bordet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregw gregw gregw approved these changes

@joakime joakime joakime left review comments

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Jetty 10.0.7/11.0.7 FROZEN
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show message if JVM args are present but new JVM is spawned based on active modules
3 participants
@sbordet @joakime @gregw