[GitHub] [zookeeper] abidaJustice commented on a change in pull request #1637: ZOOKEEPER-4241: Change log level without restarting zookeeper

[GitBox <gi...@apache.org>]

abidaJustice commented on a change in pull request #1637:
URL: https://github.com/apache/zookeeper/pull/1637#discussion_r612097003



##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/log/Log4jConfigWatcherTest.java
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.log;
+
+import java.io.BufferedWriter;
+import java.io.File;
+import java.io.FileInputStream;
+import java.io.FileWriter;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.List;
+import org.apache.commons.io.IOUtils;
+import org.apache.zookeeper.test.ClientBase;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class Log4jConfigWatcherTest {
+    private static String testLogFilePath;
+    private static final String zookeeperLogLevel = "zookeeper.root.logger=";
+    private static final String LOG_4_J_WATCHER_LOG_DIR = "log4j.watcher.log.dir";
+
+    @Before
+    public void setUp() throws Exception {
+        File tempDir = ClientBase.createEmptyTestDir();
+        testLogFilePath = tempDir.getPath() + "/zookeeper.log";
+        System.setProperty(LOG_4_J_WATCHER_LOG_DIR, tempDir.getPath());
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        System.clearProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION_WATCH);
+        System.clearProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION);
+        System.clearProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION_WATCH_INTERVAL);
+        System.clearProperty(LOG_4_J_WATCHER_LOG_DIR);
+        File dir = new File(testLogFilePath);
+        dir.delete();
+    }
+
+    @Test
+    public void testLogLevelChangeWithoutRestartingJVM() throws Exception {
+        System.setProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION_WATCH, "true");
+        System
+            .setProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION, "log4j-watcher-log4j.properties");
+        System.setProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION_WATCH_INTERVAL, "2000");
+        Log4jConfigWatcher.watchLog4jConfiguration();
+
+        // Changed the log level to WARN
+        alterLogConfigFile("/" + System.getProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION),
+            zookeeperLogLevel, zookeeperLogLevel + "WARN, ROLLINGFILE");
+        // Waiting for some time so configuration event is triggered and changes reloaded into memory
+        Thread.sleep(5000);
+        Logger LOG = LoggerFactory.getLogger(Log4jConfigWatcherTest.class);
+        LOG.warn("Warn level log is present in initial Configuration");
+        LOG.info("Info level log is present in initial Configuration");
+        Assert.assertTrue(checkTargetInLogFile(testLogFilePath,
+            "Warn level log is present in initial Configuration"));
+        Assert.assertFalse(checkTargetInLogFile(testLogFilePath,
+            "Info level log is present in initial Configuration"));
+
+        // Changed the log level to INFO
+        alterLogConfigFile("/" + System.getProperty(Log4jConfigWatcher.LOG4J_CONFIGURATION),
+            zookeeperLogLevel, zookeeperLogLevel + "INFO, ROLLINGFILE");
+        // Waiting for some time so configuration event is triggered and changes reloaded into memory
+        Thread.sleep(5000);
+        LOG.info("Info level log is present after altering the Configuration");
+        Assert.assertTrue(checkTargetInLogFile(testLogFilePath,
+            "Info level log is present after altering the Configuration"));
+    }
+
+    private void alterLogConfigFile(String log4jConfigurationFile, String key, String value)
+        throws IOException {
+        InputStream inputStream = null;
+        BufferedWriter bw = null;
+        try {
+            // log4j configuration file is modified from classpath, not from the resource folder
+            inputStream = getClass().getResourceAsStream(log4jConfigurationFile);

Review comment:
       I detect that this code is problematic. According to the [Bad practice (BAD_PRACTICE)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#bad-practice-bad-practice), [UI: Usage of GetResource may be unsafe if class is extended (UI_INHERITANCE_UNSAFE_GETRESOURCE)](https://spotbugs.readthedocs.io/en/stable/bugDescriptions.html#ui-usage-of-getresource-may-be-unsafe-if-class-is-extended-ui-inheritance-unsafe-getresource).
   Calling this.getClass().getResource(...) could give results other than expected if this class is extended by a class in another package.
   




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org