-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Jetty12 removal of all property is causing possible migration difference from some customer using external libs #11499
Comments
Starting in Jetty 10, Jetty uses slf4j entirely. There are no Jetty logging classes anymore in Jetty 10 onwards. |
I looks like the logic in https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L319-L322 is attempting to figure out if jetty logging is present. But why? |
Back in Jetty 9, here's how that property was being used. __logClass = __props.getProperty("org.eclipse.jetty.util.log.class", "org.eclipse.jetty.util.log.Slf4jLog"); As you can see if the property is unset, it defaults to Also, the fact that your code is only attempting to access it via a System.getProperty() is also bizarre and broken. |
I'm at a loss on how you got that value set as a System property in the first place. Looking at the branches
In short, aside from the |
I understand. jetty.project/jetty-core/jetty-slf4j-impl/src/test/java/org/eclipse/jetty/logging/Slf4jEffort.java Line 167 in 369d9f7
|
I can as well do a PR in the 3p library, but we would still have GAE customers using java21/jetty12 with possibly old deps and they would see different app behavior, making migration to newer GAE runtime more difficult due to internal changes affecting external app behavior. |
If the goal of that line is to produce a warning (in a System.err) if found during the Slf4jEffort utility class (not a runtime class in Jetty). Still doesn't help you with the System.getProperty call you have in your code.
Now I'm confused in a different way. |
Also, moving entirely to slf4j is a good thing, even on Jetty 6/7/8/9 as slf4j will work there as well. Perhaps a better solution would be a new artifact that is the old school Jetty logging, aka |
I see! Anyway, what I think I'll do on the GAE side is define this prop with value "unused" if it is not set before. |
Here's a quick example of slf4j ... graph TD;
lib-using-log4j1-api --> log4j-over-slf4j-bridge;
lib-using-log4j2-api --> log4j2-to-slf4j-bridge;
lib-using-commons-logging-api --> jcl-over-slf4j-bridge;
lib-using-java-util-logging-api --> jul-to-slf4j-bridge;
lib-using-slf4j-api --> slf4j-api;
log4j-over-slf4j-bridge --> slf4j-api;
log4j2-to-slf4j-bridge --> slf4j-api;
jcl-over-slf4j-bridge --> slf4j-api;
jul-to-slf4j-bridge --> slf4j-api;
slf4j-api --> slf4j-jcl-impl;
slf4j-api --> logback;
slf4j-api --> jetty-slf4j-impl;
slf4j-api --> log4j1-impl;
slf4j-api --> log4j2-impl;
slf4j-api --> java.util.logging;
slf4j-api --> android-logging;
What we can do is create a |
I have opened GoogleCloudPlatform/appengine-java-standard#100 to resolve this, so I will close this issue. The system property was set by the |
This is only for AppEngine customer using Jetty12 instead of Jetty9.
Some external library is still testing appengine path with the presence of this property being not NULL: "org.eclipse.jetty.util.log.class"
But it was removed in Jetty12 code (other than in
jetty.project/jetty-core/jetty-slf4j-impl/src/test/java/org/eclipse/jetty/logging/Slf4jEffort.java
Line 167 in 369d9f7
We could have the Jetty12 code base to define it again with a value "unused" and set it up when the jetty legacy flag is used (which is used for AppEngine code: com.google.apphosting.runtime.jetty94.LEGACY_MODE
So this bug would be for Jetty to treat the value of unused the same as being a null value in the jetty link above.
See usage of this property in
https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L68
and
https://github.com/googleapis/google-auth-library-java/blob/main/oauth2_http/java/com/google/auth/oauth2/DefaultCredentialsProvider.java#L321
The text was updated successfully, but these errors were encountered: