Github user anew commented on a diff in the pull request:
https://github.com/apache/twill/pull/14#discussion_r86287397
--- Diff:
twill-yarn/src/test/java/org/apache/twill/yarn/LogLevelChangeTestRun.java ---
@@ -0,0 +1,249 @@
+/*
+ * 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.twill.yarn;
+
+import com.google.common.base.Stopwatch;
+import com.google.common.collect.ImmutableMap;
+import org.apache.twill.api.AbstractTwillRunnable;
+import org.apache.twill.api.ResourceReport;
+import org.apache.twill.api.TwillApplication;
+import org.apache.twill.api.TwillController;
+import org.apache.twill.api.TwillRunResources;
+import org.apache.twill.api.TwillSpecification;
+import org.apache.twill.api.logging.LogEntry;
+import org.apache.twill.api.logging.PrinterLogHandler;
+import org.apache.twill.common.Threads;
+import org.junit.Assert;
+import org.junit.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.PrintWriter;
+import java.util.Map;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
+/**
+ * Test changing log level for a twill runnable.
+ */
+public class LogLevelChangeTestRun extends BaseYarnTest {
+ public static final Logger LOG =
LoggerFactory.getLogger(LogLevelChangeTestRun.class);
+
+ /**
+ * Twill runnable.
+ */
+ public static final class LogLevelTestRunnable extends
AbstractTwillRunnable {
+ public static final Logger LOG =
LoggerFactory.getLogger(LogLevelChangeTestRun.LogLevelTestRunnable.class);
+
+ private volatile Thread runThread;
+
+ @Override
+ public void run() {
+ this.runThread = Thread.currentThread();
+
+ // check if the initial log level is DEBUG
+ Assert.assertTrue(LOG.isDebugEnabled() && !LOG.isTraceEnabled());
+
+ int i = 0;
+ while (!Thread.interrupted()) {
+ if (i == 0 && !LOG.isDebugEnabled()) {
+ // check if the log level is changed to INFO
+ Assert.assertTrue(LOG.isInfoEnabled());
+ i++;
+ }
+ if (i == 1 && !LOG.isInfoEnabled()) {
+ // check if the log level is changed to WARN
+ Assert.assertTrue(LOG.isWarnEnabled());
+ i++;
+ }
+
+ try {
+ TimeUnit.MILLISECONDS.sleep(100);
+ } catch (InterruptedException e) {
+ break;
+ }
+ }
+ }
+
+ @Override
+ public void stop() {
+ if (runThread != null) {
+ runThread.interrupt();
+ }
+ }
+ }
+
+ /**
+ * Second runnable.
+ */
+ public static final class LogLevelTestSecondRunnable extends
AbstractTwillRunnable {
+ public static final Logger LOG =
LoggerFactory.getLogger(LogLevelChangeTestRun.LogLevelTestSecondRunnable.class);
+
+ private volatile Thread runThread;
+
+ @Override
+ public void run() {
+ this.runThread = Thread.currentThread();
+
+ // check if the initial log level is DEBUG
+ Assert.assertTrue(LOG.isDebugEnabled() && !LOG.isTraceEnabled());
+
+ int i = 0;
+ while (!Thread.interrupted()) {
+ if (i == 0 && !LOG.isDebugEnabled()) {
+ // check if the log level is changed to INFO
+ Assert.assertTrue(LOG.isInfoEnabled());
+ i++;
+ }
+ if (i == 1 && LOG.isDebugEnabled()) {
+ // check if the log level is changed to TRACE
+ Assert.assertTrue(LOG.isTraceEnabled());
+ i++;
+ }
+
+ try {
+ TimeUnit.MILLISECONDS.sleep(100);
+ } catch (InterruptedException e) {
+ break;
+ }
+ }
+ }
+
+ @Override
+ public void stop() {
+ if (runThread != null) {
+ runThread.interrupt();
+ }
+ }
+ }
+
+ /**
+ * A test TwillApplication to test setting log level to DEBUG.
+ */
+ public static final class LogLevelTestApplication implements
TwillApplication {
+
+ @Override
+ public TwillSpecification configure() {
+ return TwillSpecification.Builder.with()
+ .setName("LogLevelChangeTest")
+ .withRunnable()
+ .add(LogLevelTestRunnable.class.getSimpleName(), new
LogLevelTestRunnable()).noLocalFiles()
+ .add(LogLevelTestSecondRunnable.class.getSimpleName(), new
LogLevelTestSecondRunnable()).noLocalFiles()
+ .anyOrder()
+ .build();
+ }
+
+ }
+
+ @Test
+ public void testChangeLogLevel() throws Exception {
+ YarnTwillRunnerService runner = getTwillRunner();
+ runner.start();
+
+ // Set log level to DEBUG
+ TwillController controller = runner.prepare(new
LogLevelTestApplication())
+ .setLogLevel(LogEntry.Level.DEBUG)
+ .addLogHandler(new PrinterLogHandler(new PrintWriter(System.out)))
+ .start();
+
+ // Lets wait until the service is running
+ final CountDownLatch running = new CountDownLatch(1);
+ controller.onRunning(new Runnable() {
+ @Override
+ public void run() {
+ running.countDown();
+ }
+ }, Threads.SAME_THREAD_EXECUTOR);
+ Assert.assertTrue(running.await(200, TimeUnit.SECONDS));
+
+ // assert that log level is DEBUG
+ waitForLogLevel(controller, LogLevelTestRunnable.class.getSimpleName(),
--- End diff --
it seems that all you are testing here is that the log level in the
resources have changed. That, however, represents the log level that was sent
to the runnable, but we don't know whether it was actually applied. To truly
validate, the runnable should emit log messages, and the test should validate
that after changing the log level, the level of messages that actually get
collected has changed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
