[MODULES-447] Make the ModuleLoggerFinder.activate() method public. A… #329
Conversation
| @@ -88,7 +89,7 @@ public System.Logger getLogger(final String name, final java.lang.Module module) | |||
| * | |||
| * @param cl the class loader to use | |||
| */ | |||
| static void activate(final ClassLoader cl) { | |||
| public static void activate(final ClassLoader cl) { | |||
There was a problem hiding this comment.
I'd rather not expose this API; what is the use case?
There was a problem hiding this comment.
There are cases when, for example the WildFly Bootable JAR, we don't use org.jboss.modules.Main as the entry point so activate() is never invoked. It would be the responsibility of those callers which do not launch through the main entry point to invoke this. I do realize that is a bit odd. I'm definitely open to other options.
wildfly/wildfly-core#5882 is what I'm doing in WildFly Core for now.
There was a problem hiding this comment.
Reflection is probably still best for now. What we need to do is to create an embedding API in JBoss Modules and have a little bit more of a control over the bootstrapping process. Right now most important setup tasks are hard-coded in the main method. So ideally, we'd have a bootstrap API with error checking (e.g. "hey you already bootstrapped this once") and then the main method would reuse it. Maybe we could do some kind of design session on this in the near future.
There was a problem hiding this comment.
That makes a lot of sense to me. I'll pull the part of making the API public out, but I do think having the queue length somewhat auto-enable it makes some sense. What do you think of that part @dmlloyd?
There was a problem hiding this comment.
Having a maximum queue length does make sense (we've done similar things in other contexts to avoid queue runaway). But I don't know if activation is the right thing to do in response to hitting the maximum. It seems more like we're just hoping that some large-ish queue depth represents that we've waited "long enough" for things to go through, doesn't it?
There was a problem hiding this comment.
Yes, the number is completely arbitrary for sure. I guess it would likely be strange to dump a bunch of logs. The idea of activating it was just to have the System.Logger's work rather than loosing logs.
There was a problem hiding this comment.
I just discovered there's a JIRA for it already: https://issues.redhat.com/browse/MODULES-367
There was a problem hiding this comment.
Goof find @ropalka. I've linked the two JIRA's together.
…lso, activate the ModuleLoggerFinder if a threshold is hit. https://issues.redhat.com/browse/MODULES-447 Signed-off-by: James R. Perkins <jperkins@redhat.com>
|
I've updated this and #328 to both just activate logging if the threshold is hit. I feel like for logging we need to activate, but I also understand it might make the log messages look weird. For example, the date will definitely not be that friendly. However, they are log messages and I don't think we can just discard them. We have to do something. Another option might be to add the timestamp to the |
|
can we merge this if it is what we wanted ? thanks |
|
We are not likely going to use this solution. Discussion is ongoing regarding a potential bootstrap API which would be the replacement. It's not fully decided yet though. |
|
FWIW this is now just a solution which drains the queue if it reaches an arbitrary size. We could make the size configurable with a system property. |
…lso, activate the ModuleLoggerFinder if a threshold is hit.
https://issues.redhat.com/browse/MODULES-447
Upstream #328