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
Log4 j2 913 - Add support for HTTP/S configuration monitoring. Support Spring Cloud Config #253
Conversation
…nfig LOG4J2-913 - App works with Cloud Config LOG4j2-913 - Integration with Spring Cloud Config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a nice feature overall. Some various code review and API concerns above.
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Show resolved
Hide resolved
* @param url the URL where the input stream originated | ||
* @param lastModified when the source was last modified. | ||
*/ | ||
public ConfigurationSource(final InputStream stream, final URL url, long lastModified) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'll note here that for 3.x, we should certainly be redoing this API. It exposes way too much internal state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good luck with that. All this information has to be passed from one configuration to another to allow reconfiguration to occur.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've considered in the past porting over or implementing a subset of the Resource
interface from Spring. Do you think such an API would be more useful than having to inspect the ConfigurationSource union-like class?
log4j-core/src/main/java/org/apache/logging/log4j/core/config/HttpWatcher.java
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/net/ssl/SslConfigurationFactory.java
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/util/Watcher.java
Show resolved
Hide resolved
log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java
Show resolved
Hide resolved
log4j-core/src/test/java/org/apache/logging/log4j/core/util/WatchHttpTest.java
Show resolved
Hide resolved
...java/org/apache/logging/log4j/spring/cloud/config/client/Log4j2CloudConfigLoggingSystem.java
Show resolved
Hide resolved
.../org/apache/logging/log4j/spring/cloud/config/server/controller/Log4jResourceController.java
Show resolved
Hide resolved
.../org/apache/logging/log4j/spring/cloud/config/server/controller/Log4jResourceController.java
Show resolved
Hide resolved
@@ -50,6 +50,7 @@ private CompositeTriggeringPolicy(final TriggeringPolicy... triggeringPolicies) | |||
@Override | |||
public void initialize(final RollingFileManager manager) { | |||
for (final TriggeringPolicy triggeringPolicy : triggeringPolicies) { | |||
LOGGER.debug("Initializing triggering policy ", triggeringPolicy.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I don't think toString isn't necessary here, the logger will get the string value if debug is enabled
For your initial review. I still need to update the web site and look into spring cloud push notifications, so don't merge this yet.