vy commented on code in PR #3501:
URL: https://github.com/apache/logging-log4j2/pull/3501#discussion_r1995039662
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerTest.java:
##########
@@ -544,6 +544,30 @@ void testReconfiguration(final LoggerContext context)
throws Exception {
assertNotSame(newConfig, oldConfig, "Reconfiguration failed");
}
+ @Test
+ void testReconfigurationMonitorUris(final LoggerContext context) throws
Exception {
+ final Configuration oldConfig = context.getConfiguration();
+ final int MONITOR_INTERVAL_SECONDS = 5;
+ final File file = new
File("target/test-classes/org/apache/logging/log4j/core/net/ssl/keyStore.p12");
+ final long orig = file.lastModified();
+ final long newTime = orig + 10000;
+ assertTrue(file.setLastModified(newTime), "setLastModified should have
succeeded.");
+ TimeUnit.SECONDS.sleep(MONITOR_INTERVAL_SECONDS + 1);
+ for (int i = 0; i < 17; ++i) {
+ logger.debug("Reconfigure");
+ }
+ Thread.sleep(100);
+ for (int i = 0; i < 20; i++) {
+ if (context.getConfiguration() != oldConfig) {
+ break;
+ }
+ Thread.sleep(50);
+ }
Review Comment:
Could you use Awaitility instead of looping with sleep, please?
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/builder/api/package-info.java:
##########
@@ -20,7 +20,7 @@
* @since 2.4
*/
@Export
-@Version("2.20.1")
+@Version("2.21.0")
Review Comment:
Always target the version this feature will (supposedly) be shipped with:
```suggestion
@Version("2.25.0")
```
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/builder/impl/BuiltConfiguration.java:
##########
@@ -148,9 +154,28 @@ public void setShutdownTimeoutMillis(final long
shutdownTimeoutMillis) {
}
public void setMonitorInterval(final int intervalSeconds) {
+ initializeMonitoring(intervalSeconds, "");
+ }
+
+ public void initializeMonitoring(final int intervalSeconds, final String
monitorUris) {
Review Comment:
I think this should be `Set<URI> monitorUris`
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java:
##########
@@ -275,6 +275,14 @@ protected void initializeWatchers(
final Reconfigurable reconfigurable,
final ConfigurationSource configSource,
final int monitorIntervalSeconds) {
+ initializeWatchers(reconfigurable, configSource,
Collections.emptySet(), monitorIntervalSeconds);
+ }
+
+ protected void initializeWatchers(
+ final Reconfigurable reconfigurable,
+ final ConfigurationSource configSource,
+ final Collection<Source> auxiliarySources,
Review Comment:
Why don't we just provide a single `Set<Source> monitorSources`?
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/PropertiesFileConfigTest.java:
##########
@@ -64,4 +64,25 @@ void testReconfiguration(final LoggerContext context) throws
Exception {
} while (newConfig == oldConfig && loopCount++ < 5);
assertNotSame(newConfig, oldConfig, "Reconfiguration failed");
}
+
+ @Test
Review Comment:
We can remove this if we can have a dedicated `MonitorUriTest` I hinted
earlier.
##########
log4j-core-test/src/test/resources/log4j-test2.xml:
##########
@@ -15,7 +15,7 @@
~ See the License for the specific language governing permissions and
~ limitations under the License.
-->
-<Configuration status="OFF" name="XMLConfigTest" monitorInterval="1">
+<Configuration status="OFF" name="XMLConfigTest" monitorInterval="1"
monitorUris="target/test-classes/org/apache/logging/log4j/core/net/ssl/keyStore.p12">
Review Comment:
I'm reading through the changes, I don't know if it is supported, but please
use a dedicated element instead:
```
<Configuration monitorInterval="1">
<MonitorUris>
<Uri>file:///path/to/file</Uri> <!-- Note that this needs to be a URI,
not just a path! -->
<MonitorUris>
<Configuration>
```
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/LoggerTest.java:
##########
@@ -544,6 +544,30 @@ void testReconfiguration(final LoggerContext context)
throws Exception {
assertNotSame(newConfig, oldConfig, "Reconfiguration failed");
}
+ @Test
+ void testReconfigurationMonitorUris(final LoggerContext context) throws
Exception {
+ final Configuration oldConfig = context.getConfiguration();
+ final int MONITOR_INTERVAL_SECONDS = 5;
+ final File file = new
File("target/test-classes/org/apache/logging/log4j/core/net/ssl/keyStore.p12");
+ final long orig = file.lastModified();
+ final long newTime = orig + 10000;
+ assertTrue(file.setLastModified(newTime), "setLastModified should have
succeeded.");
+ TimeUnit.SECONDS.sleep(MONITOR_INTERVAL_SECONDS + 1);
Review Comment:
Ugh! Can we do something better?
### Proposal
1. Use a dedicated new test class (e.g., `MonitorUriTest`)
1. Create a random file (using `@TempDir` from JUnit)
1. [Programmatically create a
configuration](https://logging.apache.org/log4j/2.x/manual/customconfig.html#ConfigurationBuilder)
pointing to this file in a `MonitorUri` component
1. Create a `LoggerContext` from this configuration
1. Modify the random file
1. Verify that the configuration is reloaded (I presume you can find
examples in existing tests on how to perform this check.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
