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

Issue #5950 - remove WebAppClassLoader logging during loadClass #5957

Merged

Conversation

joakime
Copy link
Contributor

@joakime joakime commented Feb 8, 2021

Signed-off-by: Joakim Erdfelt [email protected]

@joakime joakime requested a review from gregw February 8, 2021 19:03
@joakime joakime self-assigned this Feb 8, 2021
@janbartel
Copy link
Contributor

Such a pity this seems to be necessary: when things are really going wrong it was so helpful to turn on debug logging for the WebAppClassLoader and find out how classes were being loaded (or not).

@olamy
Copy link
Member

olamy commented Feb 9, 2021

@joakime @janbartel if it's so helpful could we have something such -Djetty.classloader.debug=true and replace those LOG.debug with System.err/out?
Or something such

private static final boolean DEBUG = System.getBoolean("jetty.classloader.debug")
then in code where we need it
if (DEBUG) 
   System.out/err
or (maybe more complicated)
  org.eclipse.jetty.util.log.Logger.getLogger(...).debug("")

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 other than a few niggles.

{
LOG.warn(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should be e.printStackTrace() to replace warnings

@gregw
Copy link
Contributor

gregw commented Feb 9, 2021

Is it possible to test if -verbose:class is set, then we could do some stderr logging if it is? But I'm still good with this and we can add in stderr logging if/when necessary

@joakime joakime marked this pull request as draft February 9, 2021 12:11
@joakime
Copy link
Contributor Author

joakime commented Feb 9, 2021

Sorry, this isn't ready.
There's still logging occurring during load class.

@joakime joakime marked this pull request as ready for review February 15, 2021 18:21
@joakime
Copy link
Contributor Author

joakime commented Feb 15, 2021

Sorry, the draft status was in error.
The extra logging I saw was not during loadClass.
Will merge shortly.

@joakime joakime merged commit 64c7fe0 into jetty-9.4.x Feb 15, 2021
@joakime joakime deleted the jetty-9.4.x-5950-remove-logging-during-classloading branch February 15, 2021 18:22
@jglick
Copy link
Contributor

jglick commented Feb 25, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants