[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/twill/pull/14


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r91846095
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -384,6 +408,24 @@ private void setEnv(String runnableName, Map env, boolean overwr
 }
   }
 
+  private void saveLogLevels(LogEntry.Level level) {
+level = level == null ? LogEntry.Level.INFO : level;
+Map appLogLevels = new HashMap<>();
+appLogLevels.put(Logger.ROOT_LOGGER_NAME, level.name());
+for (String runnableName : twillSpec.getRunnables().keySet()) {
+  this.logLevels.put(runnableName, appLogLevels);
+}
+  }
+
+  private void saveLogLevels(String runnableName, Map logLevels) {
+Map newLevels = new HashMap<>();
+for (Map.Entry entry : logLevels.entrySet()) {
+  Preconditions.checkArgument(entry.getValue() != null, "Log level 
cannot be null for logger {}", entry.getKey());
+  newLevels.put(entry.getKey(), entry.getValue().name());
+}
+this.logLevels.get(runnableName).putAll(newLevels);
--- End diff --

What I meant was, since the method is "set", it should be setting / 
overwriting, not appending. And I think it's ok for the user to gather and pass 
in the map.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r91845224
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,45 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
+
+  /**
+   * Set the log levels for requested logger names for a {@link 
TwillRunnable}.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevelsForRunnable The {@link Map} contains the requested 
logger name and log level that
+   * need to be changed.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(String runnableName,
+   Map logLevelsForRunnable);
+
+  /**
+   * Reset the log levels of all runnables to the default log level, 
{@code LogEntry.Level.INFO}.
+   *
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done.
+   */
+  Future resetLogLevels();
--- End diff --

Changed the value in `Future` to be the loggerNames passed through this 
methods. Empty means resetting for all loggers


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r91844825
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -384,6 +408,24 @@ private void setEnv(String runnableName, Map env, boolean overwr
 }
   }
 
+  private void saveLogLevels(LogEntry.Level level) {
+level = level == null ? LogEntry.Level.INFO : level;
--- End diff --

Shouldn't default it to `INFO` at all. It should be treated the same as 
`null` is provided as the log level value as in the other `saveLogLevels` 
method, which will throw `IllegalArgumentException`. Also, rename the `level` 
parameter to `rootLogLevel` to be clear. 


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90134835
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillService.java ---
@@ -197,16 +198,16 @@ protected final void shutDown() throws Exception {
 }
   }
 
+  protected final OperationFuture updateLiveNode() {
--- End diff --

Add a javadoc for protected method as it is meant to be called by the child 
class.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90134323
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,45 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
+
+  /**
+   * Set the log levels for requested logger names for a {@link 
TwillRunnable}.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevelsForRunnable The {@link Map} contains the requested 
logger name and log level that
+   * need to be changed.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(String runnableName,
+   Map logLevelsForRunnable);
+
+  /**
+   * Reset the log levels of all runnables to the default log level, 
{@code LogEntry.Level.INFO}.
--- End diff --

Is the default always `INFO`? It's better not to say that.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r91844729
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,48 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
+
+  /**
+   * Set the log levels for requested logger names for a {@link 
TwillRunnable}.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevelsForRunnable The {@link Map} contains the requested 
logger name and log level that
+   * need to be changed.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(String runnableName,
+   Map logLevelsForRunnable);
+
+  /**
+   * Reset the log levels of all runnables.
+   * The log levels will be the same as when the runnables start up.
+   *
--- End diff --

Missed the `@param loggerNames` doc.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90134468
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,45 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
+
+  /**
+   * Set the log levels for requested logger names for a {@link 
TwillRunnable}.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevelsForRunnable The {@link Map} contains the requested 
logger name and log level that
+   * need to be changed.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(String runnableName,
+   Map logLevelsForRunnable);
+
+  /**
+   * Reset the log levels of all runnables to the default log level, 
{@code LogEntry.Level.INFO}.
+   *
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done.
+   */
+  Future resetLogLevels();
--- End diff --

What's the `String` inside the `Future` represents? If there is nothing 
actually returned from the `Future.get()`, you can specify the return type as 
`Future` and returns `null` when `Future.get()` is called.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r91844834
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -151,8 +152,9 @@
 this.reservedMemory = 
yarnConfig.getInt(Configs.Keys.JAVA_RESERVED_MEMORY_MB,
 
Configs.Defaults.JAVA_RESERVED_MEMORY_MB);
 this.extraOptions = extraOptions;
-this.logLevel = logLevel;
 this.classAcceptor = new ClassAcceptor();
+this.logLevel = logLevel;
--- End diff --

Shouldn't have this field anymore. Every log level info should be in the 
`logLevels` map


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90768827
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerService.java
 ---
@@ -51,29 +63,36 @@
   private final ClassLoader classLoader;
   private final BasicTwillContext context;
   private final ContainerLiveNodeData containerLiveNodeData;
+  private final Map oldLogLevels;
+  private final Map defaultLogLevels;
   private ExecutorService commandExecutor;
   private TwillRunnable runnable;
 
   public TwillContainerService(BasicTwillContext context, ContainerInfo 
containerInfo, ZKClient zkClient,
RunId runId, TwillRunnableSpecification 
specification, ClassLoader classLoader,
-   Location applicationLocation) {
+   Location applicationLocation, Map defaultLogLevels,
+   Map logLevels) {
 super(zkClient, runId, applicationLocation);
 
 this.specification = specification;
 this.classLoader = classLoader;
-this.containerLiveNodeData = createLiveNodeData(containerInfo);
+this.defaultLogLevels = ImmutableMap.copyOf(defaultLogLevels);
+this.oldLogLevels = Collections.synchronizedMap(new 
HashMap<>(defaultLogLevels));
--- End diff --

I see. Seems ok. 


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90767592
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerService.java
 ---
@@ -51,29 +63,36 @@
   private final ClassLoader classLoader;
   private final BasicTwillContext context;
   private final ContainerLiveNodeData containerLiveNodeData;
+  private final Map oldLogLevels;
+  private final Map defaultLogLevels;
   private ExecutorService commandExecutor;
   private TwillRunnable runnable;
 
   public TwillContainerService(BasicTwillContext context, ContainerInfo 
containerInfo, ZKClient zkClient,
RunId runId, TwillRunnableSpecification 
specification, ClassLoader classLoader,
-   Location applicationLocation) {
+   Location applicationLocation, Map defaultLogLevels,
+   Map logLevels) {
 super(zkClient, runId, applicationLocation);
 
 this.specification = specification;
 this.classLoader = classLoader;
-this.containerLiveNodeData = createLiveNodeData(containerInfo);
+this.defaultLogLevels = ImmutableMap.copyOf(defaultLogLevels);
+this.oldLogLevels = Collections.synchronizedMap(new 
HashMap<>(defaultLogLevels));
--- End diff --

When you reset, you cannot reset the log level for a particular logger. It 
will reset the log levels for all loggers which are changed during runtime. So 
if you call reset, it will reset the log level of ROOT to INFO, and the 
particular logger to the log level when the runnable starts. If it was null 
initially, its log level will be following the ROOT log level, INFO


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90767420
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,45 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
+
+  /**
+   * Set the log levels for requested logger names for a {@link 
TwillRunnable}.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevelsForRunnable The {@link Map} contains the requested 
logger name and log level that
+   * need to be changed.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(String runnableName,
+   Map logLevelsForRunnable);
+
+  /**
+   * Reset the log levels of all runnables to the default log level, 
{@code LogEntry.Level.INFO}.
+   *
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done.
+   */
+  Future resetLogLevels();
+
+  /**
+   * Reset the log levels of the given runnable to the log levels when .
--- End diff --

My bad for not updating the comments. The `resetLogLevels` will reset the 
log level of a runnable to the time when it starts up. This means it will reset 
the log level to what `logback.xml` is + the log level requested from 
`TwillPreparer`


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90767247
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerService.java
 ---
@@ -51,29 +63,36 @@
   private final ClassLoader classLoader;
   private final BasicTwillContext context;
   private final ContainerLiveNodeData containerLiveNodeData;
+  private final Map oldLogLevels;
+  private final Map defaultLogLevels;
   private ExecutorService commandExecutor;
   private TwillRunnable runnable;
 
   public TwillContainerService(BasicTwillContext context, ContainerInfo 
containerInfo, ZKClient zkClient,
RunId runId, TwillRunnableSpecification 
specification, ClassLoader classLoader,
-   Location applicationLocation) {
+   Location applicationLocation, Map defaultLogLevels,
+   Map logLevels) {
 super(zkClient, runId, applicationLocation);
 
 this.specification = specification;
 this.classLoader = classLoader;
-this.containerLiveNodeData = createLiveNodeData(containerInfo);
+this.defaultLogLevels = ImmutableMap.copyOf(defaultLogLevels);
+this.oldLogLevels = Collections.synchronizedMap(new 
HashMap<>(defaultLogLevels));
--- End diff --

can you explain what oldLogLevels is for? My understanding is this:
- when a log level is updated, you save previous level in oldLogLevels
- when the log level is reset, you restore the log level from oldLogLevels
Is that correct?

If so, then consider this scenario:
- root log level is INFO
- I debug a problem, so I set the root log level to DEBUG
- I set the log level for one logger to TRACE
- now I found the problem. 
- I reset the log level for that particular logger
- then I set the root log level back to INFO

what will be the log level for that particular logger after all this? INFO 
or DEBUG?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90767079
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,44 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
--- End diff --

Still, the way I understand this is that it means, the message was received 
by the app master and passed on to the runnables. We have no idea whether the 
runnables have received or processed it. That can only be determined by 
examining the live node info for each runnable.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r90767209
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,45 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
+
+  /**
+   * Set the log levels for requested logger names for a {@link 
TwillRunnable}.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
+   * the specified logger.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevelsForRunnable The {@link Map} contains the requested 
logger name and log level that
+   * need to be changed.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(String runnableName,
+   Map logLevelsForRunnable);
+
+  /**
+   * Reset the log levels of all runnables to the default log level, 
{@code LogEntry.Level.INFO}.
+   *
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done.
+   */
+  Future resetLogLevels();
+
+  /**
+   * Reset the log levels of the given runnable to the log levels when .
--- End diff --

this sentence is incomplete. Does this mean restore the log level to what 
it was before the last setLogLevel()? Or before setLogLevel was called for the 
first time? Or to what it was in the original spec?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87877029
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,30 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
--- End diff --

We check it using `Preconditions.checkArgument`, so an exception will be 
thrown for null log level for root logger.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87875415
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/container/TwillContainerService.java
 ---
@@ -56,24 +66,27 @@
 
   public TwillContainerService(BasicTwillContext context, ContainerInfo 
containerInfo, ZKClient zkClient,
RunId runId, TwillRunnableSpecification 
specification, ClassLoader classLoader,
-   Location applicationLocation) {
+   Location applicationLocation, Map logLevels) {
 super(zkClient, runId, applicationLocation);
 
 this.specification = specification;
 this.classLoader = classLoader;
-this.containerLiveNodeData = createLiveNodeData(containerInfo);
+this.containerLiveNodeData = createLiveNodeData(containerInfo,
+isLoggerContext()
+  ? logLevels : 
Collections.emptyMap());
 this.context = context;
   }
 
-  private ContainerLiveNodeData createLiveNodeData(ContainerInfo 
containerInfo) {
+  private ContainerLiveNodeData createLiveNodeData(ContainerInfo 
containerInfo,
+   Map logLevels) {
 // if debugging is enabled, log the port and register it in service 
discovery.
 String debugPort = System.getProperty("twill.debug.port");
 if (debugPort != null) {
   LOG.info("JVM is listening for debugger on port {}", debugPort);
 }
 return new ContainerLiveNodeData(containerInfo.getId(),
  
containerInfo.getHost().getCanonicalHostName(),
- debugPort);
+ debugPort, logLevels);
--- End diff --

I think we should filter out the one that has `null` value, since those are 
representing reset.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87353516
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,30 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   * The log level for a logger name can be {@code null} except for the 
root logger, which will reset the log level for
--- End diff --

What happen if the caller pass in `null` value for the root logger? Will it 
throw exception? Or will it get ignore?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87864069
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/internal/DefaultTwillRunResources.java 
---
@@ -90,8 +104,14 @@ public Integer getDebugPort() {
   }
 
   @Override
+  @Deprecated
   public Level getLogLevel() {
-return logLevel;
+return getLogLevels().get(Logger.ROOT_LOGGER_NAME);
--- End diff --

So this may return `null`, right? Or can we guarantee that it won't be 
`null` since we always know the root log level when a container starts?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87353645
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillPreparer.java 
---
@@ -227,15 +227,34 @@
   TwillPreparer addSecureStore(SecureStore secureStore);
 
   /**
-   * Set the log level for Twill applications running in a container.
+   * Set the root log level for Twill applications in all containers.
*
* @param logLevel the {@link LogEntry.Level} that should be set.
* The level match the {@code Logback} levels.
* @return This {@link TwillPreparer}.
+   * @deprecated Use {@link #setLogLevels(Map)} with logger name 
Logger.ROOT_LOGGER_NAME instead.
--- End diff --

Should be `{@link Logger#ROOT_LOGGER_NAME}`


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87863869
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillPreparer.java 
---
@@ -227,15 +227,34 @@
   TwillPreparer addSecureStore(SecureStore secureStore);
 
   /**
-   * Set the log level for Twill applications running in a container.
+   * Set the root log level for Twill applications in all containers.
*
* @param logLevel the {@link LogEntry.Level} that should be set.
* The level match the {@code Logback} levels.
* @return This {@link TwillPreparer}.
+   * @deprecated Use {@link #setLogLevels(Map)} with logger name 
Logger.ROOT_LOGGER_NAME instead.
*/
+  @Deprecated
   TwillPreparer setLogLevel(LogEntry.Level logLevel);
 
   /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return This {@link TwillPreparer}.
+   */
+  TwillPreparer setLogLevels(Map logLevels);
--- End diff --

Will this map allows `null` value? Unlike the dynamic case, it seems like 
there is no point in allowing that, which means we should validate and document 
it?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87254498
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/ServiceMain.java ---
@@ -264,6 +262,10 @@ protected String getLoggerLevel(Logger logger) {
 return "OFF";
   }
 
+  protected String getLogLevels() {
+return "";
--- End diff --

For a launch, the `TwillContainerService` is constructed before the run of 
`doMain` method, which means the we have not called `configureLogger()` method 
at that time so log level setting at constructor will not work. 


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87238505
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/internal/DefaultTwillRunResources.java 
---
@@ -129,7 +153,7 @@ public String toString() {
   ", memoryMB=" + memoryMB +
   ", host='" + host + '\'' +
   ", debugPort=" + debugPort +
-  ", logLevel=" + logLevel +
+  ", rootLogLevel=" + logLevels.get(Logger.ROOT_LOGGER_NAME) +
--- End diff --

Why only has root log level? The `toString` should includes the whole 
`logLevels` map.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87242433
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/ContainerLiveNodeData.java 
---
@@ -43,4 +51,15 @@ public String getHost() {
   public String getDebugPort() {
 return debugPort;
   }
+
+  public Map getLogLevels() {
+return logLevels;
+  }
+
+  public void updateLogLevels(Map logLevels, 
boolean isReset) {
+if (isReset) {
--- End diff --

This is not a good way overloading the responsibility of this method. 
Better have a separate method for resetting (and that mirrors the actual 
functionality being exposed through the public API).


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87244253
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
 ---
@@ -17,12 +17,14 @@
  */
 package org.apache.twill.internal;
 
+import ch.qos.logback.classic.Logger;
--- End diff --

Unused import?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87237581
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/api/TwillRunResources.java ---
@@ -58,7 +60,18 @@
 
   /**
* @return the enabled log level for the container where the runnable is 
running in.
+   * @deprecated Use {@link #getRootLogLevel()} instead.
*/
+  @Deprecated
   LogEntry.Level getLogLevel();
 
+  /**
+   * @return the enabled log level for the container where the runnable is 
running in.
+   */
+  LogEntry.Level getRootLogLevel();
--- End diff --

Same reason as above, don't introduce this method just specialized for root 
logger.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87239528
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,59 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the root log level for Twill applications in all containers.
+   *
+   * @param logLevel The log level for the root logger to change.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry
+   * the log level as a result.
+   */
+  Future setRootLogLevel(LogEntry.Level logLevel);
+
+  /**
+   * Set the root log level for a particular runnable.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevel The log level for the root logger to change.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry
+   * the log level as a result.
+   */
+  Future setRootLogLevel(String runnableName, 
LogEntry.Level logLevel);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
+
+  /**
+   * Set the log levels for requested logger names for a {@link 
TwillRunnable}.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevelsForRunnable The {@link Map} contains the requested 
logger name and log level that
+   * need to be changed.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(String runnableName,
+   Map logLevelsForRunnable);
+
+  /**
+   * Reset the log levels of all runnables to the default log level, 
{@code LogEntry.Level.INFO}.
+   *
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done.
+   */
+  Future resetLogLevelsToDefault();
+
+  /**
+   * Reset the log levels of the given runnable to the default log level, 
{@code LogEntry.Level.INFO}.
+   *
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * runnable name as the result.
+   */
+  Future resetRunnableLogLevelsToDefault(String runnableName);
--- End diff --

Also, this reset everything? So there is no way to reset only a subset of 
loggers?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87239353
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java 
---
@@ -170,6 +171,37 @@ public String apply(Set input) {
 });
   }
 
+  @Override
+  public Future setRootLogLevel(LogEntry.Level logLevel) {
+return 
sendMessage(SystemMessages.setLogLevels(ImmutableMap.of(Logger.ROOT_LOGGER_NAME,
 logLevel)), logLevel);
+  }
+
+  @Override
+  public Future setRootLogLevel(String runnableName, 
LogEntry.Level logLevel) {
+return sendMessage(SystemMessages.setLogLevels(runnableName, 
ImmutableMap.of(Logger.ROOT_LOGGER_NAME, logLevel)),
+   logLevel);
+  }
+
+  @Override
+  public Future> setLogLevels(Map logLevels) {
+return sendMessage(SystemMessages.setLogLevels(logLevels), logLevels);
+  }
+
+  @Override
+  public Future> setLogLevels(String 
runnableName,
+  Map runnableLogLevels) {
+return sendMessage(SystemMessages.setLogLevels(runnableName, 
runnableLogLevels), runnableLogLevels);
+  }
+
+  @Override
+  public Future resetLogLevelsToDefault() {
+return sendMessage(SystemMessages.resetLogLevels(null), "all");
--- End diff --

This is inconsistent with the `setLogLevels` method, which doesn't require 
explicitly passing in `null` for all runnables case.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r87244879
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/ServiceMain.java ---
@@ -264,6 +262,10 @@ protected String getLoggerLevel(Logger logger) {
 return "OFF";
   }
 
+  protected String getLogLevels() {
+return "";
--- End diff --

Why we can't set the custom log levels at launch in the same programmatic 
way as when a system message is received?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86290796
  
--- 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();
+   

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86288660
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -475,6 +496,11 @@ private void sendMessage(final String runnableName, 
final Message message,
 Futures.addCallback(controller.sendMessage(message), new 
FutureCallback() {
   @Override
   public void onSuccess(Message result) {
+//update log level information
--- End diff --

onSuccess here means that the message has been processed by the 
TwillContainerService, and the log levels have been set up. If there is some 
error setting up the log levels, an failed future with exception will be 
returned to callback, so it will go to onFailure() method


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86288445
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -575,18 +601,74 @@ private boolean removeContainerInfo(String 
containerId) {
 return false;
   }
 
+  private Map> 
copyLogLevels(Map> originalMap) {
+Map> result = new TreeMap<>();
+for (Map.Entry> entry : 
originalMap.entrySet()) {
+  Map value = new TreeMap<>();
+  for (Map.Entry valueEntry : 
entry.getValue().entrySet()) {
+value.put(valueEntry.getKey(), valueEntry.getValue());
+  }
+  result.put(entry.getKey(), value);
+}
+return result;
+  }
+
+  private void checkAndSetLogLevels(Message message, String runnableName) {
+if (message.getType() != Message.Type.SYSTEM
+  || 
!SystemMessages.LOG_LEVEL.equals(message.getCommand().getCommand())) {
+  return;
+}
+
+Map runnableLogLevels = 
convertLogLevelValuesToLogEntry(message.getCommand().getOptions());
+if (!logLevels.containsKey(runnableName)) {
+  logLevels.put(runnableName, runnableLogLevels);
+} else {
+  logLevels.get(runnableName).putAll(runnableLogLevels);
+}
+  }
+
+  private Location saveLogLevels() {
--- End diff --

We save the log levels to file because we have no way to know if there is 
message about log level change come before the launch of some containers(this 
can happen when we change instance count or restart the container). So when a 
message about log level change come, we will have it stored in the Map in 
`RunningContainers`. And if there is a container launch afterwards, we will 
check if there is update in the log levels, and localize it for the container 
if necessary.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86287618
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
 ---
@@ -89,8 +91,7 @@ public String getRmSchedulerAddr() {
 return rmSchedulerAddr;
   }
 
-  @Nullable
-  public LogEntry.Level getLogLevel() {
-return logLevel;
+  public Map> getLogLevels() {
+return logLevels;
   }
--- End diff --

ok


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

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();
+  }
 

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86287245
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -296,8 +298,42 @@ public TwillPreparer addSecureStore(SecureStore 
secureStore) {
 
   @Override
   public TwillPreparer setLogLevel(LogEntry.Level logLevel) {
+return setRootLogLevel(logLevel);
+  }
+
+  @Override
+  public TwillPreparer setRootLogLevel(LogEntry.Level logLevel) {
 Preconditions.checkNotNull(logLevel);
 this.logLevel = logLevel;
+saveLogLevels(logLevel);
+return this;
+  }
+
+  @Override
+  public TwillPreparer setRootLogLevel(String runnableName, LogEntry.Level 
logLevel) {
+setLogLevels(runnableName, ImmutableMap.of(Logger.ROOT_LOGGER_NAME, 
logLevel));
+return this;
+  }
+
+  @Override
+  public TwillPreparer setLogLevels(Map logLevels) 
{
+Preconditions.checkNotNull(logLevels);
+if (logLevels.containsKey(Logger.ROOT_LOGGER_NAME)) {
+  this.logLevel = logLevels.get(Logger.ROOT_LOGGER_NAME);
+}
+for (String runnableName : twillSpec.getRunnables().keySet()) {
+  saveLogLevels(runnableName, logLevels);
+}
+return this;
+  }
+
+  @Override
+  public TwillPreparer setLogLevels(String runnableName, Map runnableLogLeves) {
--- End diff --

typo: runnableLogLevels


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86286972
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -475,6 +496,11 @@ private void sendMessage(final String runnableName, 
final Message message,
 Futures.addCallback(controller.sendMessage(message), new 
FutureCallback() {
   @Override
   public void onSuccess(Message result) {
+//update log level information
--- End diff --

onSuccess here really means that the message has been sent to the runnable 
successfully, right? But it does not reflect the actual state of the runnable 
itself? It that on purpose?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86282861
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,44 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the root log level for Twill applications in all containers.
+   *
+   * @param logLevel The log level for the root logger to change.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry
+   * the log level as a result.
+   */
+  Future setRootLogLevel(LogEntry.Level logLevel);
+
+  /**
+   * Set the root log level for a particular runnable.
+   *
+   * @param runnableName The name of the runnable to set the log level.
+   * @param logLevel The log level for the root logger to change.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry
+   * the log level as a result.
+   */
+  Future setRootLogLevel(String runnableName, 
LogEntry.Level logLevel);
+
+  /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   *
+   * @param logLevels The {@link Map} contains the requested logger names 
and log levels that need to be set.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry the
+   * {@link Map} of log levels as the result.
+   */
+  Future> setLogLevels(Map logLevels);
--- End diff --

Yes, you can set the root log level by calling like:
`setLogLevels(ImmutableMap.of("ROOT", TRACE))`


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[anew]

Github user anew commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r86275201
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/DefaultResourceReport.java 
---
@@ -158,4 +174,31 @@ public String toString() {
   ", services=" + services +
   '}';
   }
+
+  private TwillRunResources wrapTwillRunResources(final String 
runnableName, TwillRunResources resources) {
--- End diff --

can you explain what this is for? It's not obvious to me. I think it 
deserves some commenting. 


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83519663
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/DefaultResourceReport.java 
---
@@ -39,25 +44,33 @@
   private final TwillRunResources appMasterResources;
   private final String applicationId;
   private final AtomicReference services;
+  private final Map> logLevels;
 
   public DefaultResourceReport(String applicationId, TwillRunResources 
masterResources) {
-this(applicationId, masterResources, ImmutableMap.of());
-  }
-
-  public DefaultResourceReport(String applicationId, TwillRunResources 
masterResources,
-   Map 
resources) {
-this(applicationId, masterResources, resources, 
ImmutableList.of());
+this(applicationId, masterResources, Collections.emptyMap(),
+ Collections.emptyList());
   }
 
   public DefaultResourceReport(String applicationId, TwillRunResources 
masterResources,
Map 
resources, List services) {
 this.applicationId = applicationId;
 this.appMasterResources = masterResources;
-this.usedResources = HashMultimap.create();
+this.usedResources = 
Multimaps.synchronizedSetMultimap(HashMultimap.create());
 for (Map.Entry entry : 
resources.entrySet()) {
   this.usedResources.putAll(entry.getKey(), entry.getValue());
 }
 this.services = new AtomicReference<>(services);
+this.logLevels = new ConcurrentHashMap<>();
+  }
+
+  public synchronized void setRunnableLogLevels(String runnableName, 
Map runnableLogLevels) {
--- End diff --

Also, maybe good to just return if the `runnableLogLevels` map is empty.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83519594
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/DefaultResourceReport.java 
---
@@ -39,25 +44,33 @@
   private final TwillRunResources appMasterResources;
   private final String applicationId;
   private final AtomicReference services;
+  private final Map> logLevels;
 
   public DefaultResourceReport(String applicationId, TwillRunResources 
masterResources) {
-this(applicationId, masterResources, ImmutableMap.of());
-  }
-
-  public DefaultResourceReport(String applicationId, TwillRunResources 
masterResources,
-   Map 
resources) {
-this(applicationId, masterResources, resources, 
ImmutableList.of());
+this(applicationId, masterResources, Collections.emptyMap(),
+ Collections.emptyList());
   }
 
   public DefaultResourceReport(String applicationId, TwillRunResources 
masterResources,
Map 
resources, List services) {
 this.applicationId = applicationId;
 this.appMasterResources = masterResources;
-this.usedResources = HashMultimap.create();
+this.usedResources = 
Multimaps.synchronizedSetMultimap(HashMultimap.create());
 for (Map.Entry entry : 
resources.entrySet()) {
   this.usedResources.putAll(entry.getKey(), entry.getValue());
 }
 this.services = new AtomicReference<>(services);
+this.logLevels = new ConcurrentHashMap<>();
+  }
+
+  public synchronized void setRunnableLogLevels(String runnableName, 
Map runnableLogLevels) {
--- End diff --

better name it as `updateRunnableLogLevels`.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83471666
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,34 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the root log level for Twill applications in all containers.
+   *
+   * @param logLevel The log level for the root logger to change.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry
+   * the {@link Map} of the log level as a result with ROOT as the 
logger name and the log level as value.
+   */
+  Future> setLogLevel(LogEntry.Level logLevel);
--- End diff --

Also, for the `Future`, it seems like it's better just to carry 
`LogEntry.Level`.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83472284
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/internal/DefaultTwillRunResources.java 
---
@@ -129,7 +152,7 @@ public String toString() {
   ", memoryMB=" + memoryMB +
   ", host='" + host + '\'' +
   ", debugPort=" + debugPort +
-  ", logLevel=" + logLevel +
+  ", rootLogLevel=" + getRootLogLevel() +
--- End diff --

Use the `logLevels` field instead of calling the get method to be 
consistent.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83471254
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/ResourceReport.java 
---
@@ -61,4 +63,11 @@
* @return list of services of the application master.
*/
   List getServices();
+
+  /**
+   * Get the map of the log levels enabled for the twill application.
+   *
+   * @return the map of the log level arguments of the twill application
+   */
+  Map> getLogLevels();
--- End diff --

Why have this method? The log levels are already contained inside 
individual `TwillRunResources`.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83472057
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/api/TwillRunResources.java ---
@@ -59,6 +61,16 @@
   /**
* @return the enabled log level for the container where the runnable is 
running in.
*/
+  @Deprecated
--- End diff --

Why deprecating a method, also add a `@deprecated` in the javadoc section 
to tell what's the alternative. E.g.

```
/**
 * 
 * @deprecated Use {@link #getRootLogLevel()} instead.
 */


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83471522
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,34 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the root log level for Twill applications in all containers.
+   *
+   * @param logLevel The log level for the root logger to change.
+   * @return A {@link Future} that will be completed when the set log 
level operation has been done. It will carry
+   * the {@link Map} of the log level as a result with ROOT as the 
logger name and the log level as value.
+   */
+  Future> setLogLevel(LogEntry.Level logLevel);
--- End diff --

So there is no way to set the root log level for a particular runnable?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83116829
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -951,4 +958,34 @@ public void run() {
   }
 };
   }
+
+  /**
+   * Attempt to change the log level from a runnable or all runnables.
+   *
+   * @return {@code true} if the message requests changing log levels and 
{@code false} otherwise.
+   */
+  private boolean handleLogLevelChange(final Message message, final 
Runnable completion) {
--- End diff --

Seems like the `final` is unnecessary.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83112064
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/utils/LogLevelUtil.java ---
@@ -0,0 +1,56 @@
+/*
+ * 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.internal.utils;
+
+import com.google.common.collect.Maps;
+import org.apache.twill.api.logging.LogEntry;
+
+import java.util.Map;
+
+/**
+ * Collections of helper functions for log level change.
+ */
+public final class LogLevelUtil {
+
+  /**
+   * Convert the log level argument value type to string.
+   */
+  public static Map 
convertLogLevelValuesToString(Map logLevels) {
--- End diff --

No need to have this method. Just do a

```Maps.transformValues(logLevels, Functions.toStringFunction())```

when needed.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83117233
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -951,4 +958,34 @@ public void run() {
   }
 };
   }
+
+  /**
+   * Attempt to change the log level from a runnable or all runnables.
+   *
+   * @return {@code true} if the message requests changing log levels and 
{@code false} otherwise.
+   */
+  private boolean handleLogLevelChange(final Message message, final 
Runnable completion) {
+Message.Scope scope = message.getScope();
+if (message.getType() != Message.Type.SYSTEM ||
+  (scope != Message.Scope.RUNNABLE && scope != 
Message.Scope.ALL_RUNNABLE)) {
+  return false;
+}
+
+Command command = message.getCommand();
+if (!command.getCommand().equals(SystemMessages.LOG_LEVEL)) {
+  return false;
+}
+
+if (scope == Message.Scope.ALL_RUNNABLE) {
+  runningContainers.sendToAll(message, completion);
+} else {
+  final String runnableName = message.getRunnableName();
+  if (runnableName == null || runnableName.isEmpty() || 
!twillSpec.getRunnables().containsKey(runnableName)) {
--- End diff --

No need to check if `runnableName` is empty.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83116780
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/ApplicationMasterService.java
 ---
@@ -200,7 +201,9 @@ private RunningContainers 
initRunningContainers(ContainerId appMasterContainerId
   Integer.parseInt(System.getenv(EnvKeys.YARN_CONTAINER_MEMORY_MB)),
   appMasterHost, null, null);
 String appId = 
appMasterContainerId.getApplicationAttemptId().getApplicationId().toString();
-return new RunningContainers(appId, appMasterResources, zkClient);
+return new RunningContainers(appId, appMasterResources, zkClient,
+ twillRuntimeSpec.getLogLevels(), 
applicationLocation,
+ new 
ArrayList<>(twillSpec.getRunnables().keySet()));
--- End diff --

No need to copy since twill spec is immutable.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83118218
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/internal/appmaster/RunningContainers.java
 ---
@@ -132,16 +149,22 @@ void start(String runnableName, ContainerInfo 
containerInfo, TwillContainerLaunc
 try {
   int instanceId = getStartInstanceId(runnableName);
   RunId runId = getRunId(runnableName, instanceId);
+  Map logLevelsRunnable = 
resourceReport.getLogLevels().containsKey(runnableName) ?
+resourceReport.getLogLevels().get(runnableName) : new 
HashMap();
+  logLevel = logLevelsRunnable.containsKey(Logger.ROOT_LOGGER_NAME) ?
--- End diff --

Is `ROOT_LOGGER_NAME` a special logger name? If that's the case, we should 
be consistently using the map only. I see some other classes having both 
`rootLogLevel` and the map.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83108717
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/json/TwillRunResourcesCodec.java
 ---
@@ -41,7 +43,8 @@
   private static final String MEMORY_MB = "memoryMB";
   private static final String VIRTUAL_CORES = "virtualCores";
   private static final String DEBUG_PORT = "debugPort";
-  private static final String LOG_LEVEL = "logLevel";
+  private static final String LOG_LEVEL = "rootLogLevel";
+  private static final String LOG_LEVEL_ARGUMENTS = "logLevels";
--- End diff --

Why not call it `LOG_LEVELS`?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83083453
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/internal/DefaultTwillRunResources.java 
---
@@ -30,17 +34,33 @@
   private final int memoryMB;
   private final String host;
   private final Integer debugPort;
-  private final Level logLevel;
+  private final Map logLevels;
+  private Level rootLogLevel;
+
+  public DefaultTwillRunResources(int instanceId, String containerId, int 
cores, int memoryMB,
--- End diff --

Please add javadocs.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83089162
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/DefaultResourceReport.java 
---
@@ -149,6 +161,51 @@ public void setServices(List services) {
 return services.get();
   }
 
+  /**
+   * Get the map of the log level arguments of the twill application.
+   *
+   * @return the map of the log level arguments of the twill application
+   */
+  @Override
+  public Map> getLogLevels() {
+return logLevels;
+  }
+
+  /**
+   * Save the log levels for the twill runnable.
+   *
+   * @param runnableName name of the runnable.
+   * @param logLevels map of the log level arguments to be saved.
+   */
+  public void setLogLevels(String runnableName, Map logLevels) {
+Map newlogLevels = new 
ConcurrentHashMap<>(logLevels);
--- End diff --

So I assume there will be non-concurrent calls to this method, right? 
Otherwise one will overwrite each other. Also, if that's the case, you don't 
need a `ConcurrentHashMap` as the value in the `logLevels` map, but rather an 
immutable map is sufficient.

However, if there can be concurrent calls to this method, you'll need to 
merge the map, instead of replacing it.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83108745
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/json/TwillRunResourcesCodec.java
 ---
@@ -41,7 +43,8 @@
   private static final String MEMORY_MB = "memoryMB";
   private static final String VIRTUAL_CORES = "virtualCores";
   private static final String DEBUG_PORT = "debugPort";
-  private static final String LOG_LEVEL = "logLevel";
+  private static final String LOG_LEVEL = "rootLogLevel";
--- End diff --

Should rename to `ROOT_LOG_LEVEL` to be consistent.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r83083471
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/internal/DefaultTwillRunResources.java 
---
@@ -30,17 +34,33 @@
   private final int memoryMB;
   private final String host;
   private final Integer debugPort;
-  private final Level logLevel;
+  private final Map logLevels;
+  private Level rootLogLevel;
+
+  public DefaultTwillRunResources(int instanceId, String containerId, int 
cores, int memoryMB,
+  String host, Integer debugPort, Level 
rootLogLevel) {
+this(instanceId, containerId, cores, memoryMB, host, debugPort, 
rootLogLevel,
+ new HashMap());
+  }
+
+  public DefaultTwillRunResources(TwillRunResources twillRunResources, 
LogEntry.Level rootLogLevel,
--- End diff --

Same here.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82901837
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/utils/LogLevelUtil.java ---
@@ -0,0 +1,87 @@
+/*
+ * 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.internal.utils;
+
+import com.google.common.collect.Maps;
+import org.apache.twill.api.logging.LogEntry;
+import org.apache.twill.internal.Constants;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+
+/**
+ * Collections of helper functions for log level change.
+ */
+public final class LogLevelUtil {
+
+  /**
+   * Convert the log level argument value type to string.
+   */
+  public static Map 
convertLogLevelValuesToString(Map logLevels) {
+return Maps.transformEntries(logLevels, new 
Maps.EntryTransformer() {
+  @Override
+  public String transformEntry(String loggerName, LogEntry.Level 
level) {
+return level.name();
+  }
+});
+  }
+
+  /**
+   * Convert the log level argument type to LogEntry.Level
+   */
+  public static Map 
convertLogLevelValuesToLogEntry(Map logLevels) {
+return Maps.transformEntries(logLevels, new 
Maps.EntryTransformer() {
+  @Override
+  public LogEntry.Level transformEntry(String loggerName, String 
level) {
+return LogEntry.Level.valueOf(level);
+  }
+});
+  }
+
+  /**
+   * Get the log level arguments for a twill runnable.
+   *
+   * @param runnableName name of the runnable.
+   * @param logLevels the arguments for all runnables.
+   * @return the map of the log level arguments for the runnable, empty if 
there is no argument.
+   */
+  public static Map getLogLevelForRunnable(
--- End diff --

We shouldn't have this method as we shouldn't use `LOG_ALL_RUNNABLES` as a 
special runnable name. It's better to have the log levels populated to each 
runnable log level map when setting/changing it.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82899928
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/LogLevelChangeTestRun.java ---
@@ -0,0 +1,237 @@
+/*
+ * 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()) {
--- End diff --

Without sleeping/pausing, the thread can take up all CPU resources, causing 
starvation for other threads. On machine with single core, unit-test could fail 
due to this.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82901178
  
--- Diff: 
twill-yarn/src/main/java/org/apache/twill/yarn/YarnTwillPreparer.java ---
@@ -298,6 +299,29 @@ public TwillPreparer addSecureStore(SecureStore 
secureStore) {
   public TwillPreparer setLogLevel(LogEntry.Level logLevel) {
 Preconditions.checkNotNull(logLevel);
 this.logLevel = logLevel;
+Map logLevels = new HashMap<>();
+logLevels.put(Logger.ROOT_LOGGER_NAME, logLevel);
+saveLogLevels(Constants.SystemMessages.LOG_ALL_RUNNABLES, logLevels);
--- End diff --

Why use the `LOG_ALL_RUNNABLES` as a runnable name? We have the `twillSpec` 
in this class, so it's easy to just expand the log level settings for each 
runnable.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82898688
  
--- Diff: 
twill-yarn/src/test/java/org/apache/twill/yarn/ResourceReportTestRun.java ---
@@ -36,6 +36,7 @@
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import scala.reflect.api.Constants;
--- End diff --

unused import.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82886998
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/utils/LogLevelUtil.java ---
@@ -0,0 +1,85 @@
+/*
+ * 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.internal.utils;
+
+import com.google.common.collect.Maps;
+import org.apache.twill.api.logging.LogEntry;
+import org.apache.twill.internal.Constants;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+
+/**
+ * Collections of helper functions for log level change.
+ */
+public final class LogLevelUtil {
+
+  /**
+   * Convert the log level argument value type to string.
+   */
+  public static Map 
convertLogLevelValuesToString(Map logLevelArguments) {
--- End diff --

The methods in `LogLevelUtil` are not only used in `SystemMessages`. They 
are also used in `RunningContainers` and `TwillContainerMain`, is it good to 
have all these methods in `SystemMessages`?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[yaojiefeng]

Github user yaojiefeng commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82102988
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/AbstractTwillController.java 
---
@@ -137,7 +138,7 @@ public final synchronized ServiceDiscovered 
discoverService(String serviceName)
 
   @Override
   public final ListenableFuture restartInstances(Map runnableToInstanceIds) {
+? extends Set> runnableToInstanceIds) {
--- End diff --

I cannot change the type to Future for this method, there is a another 
method calling it which requires the return type to be a ListenableFuture


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82099632
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/utils/LogLevelUtil.java ---
@@ -0,0 +1,85 @@
+/*
+ * 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.internal.utils;
+
+import com.google.common.collect.Maps;
+import org.apache.twill.api.logging.LogEntry;
+import org.apache.twill.internal.Constants;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Set;
+import javax.annotation.Nullable;
+
+/**
+ * Collections of helper functions for log level change.
+ */
+public final class LogLevelUtil {
+
+  /**
+   * Convert the log level argument value type to string.
+   */
+  public static Map 
convertLogLevelValuesToString(Map logLevelArguments) {
+return Maps.transformEntries(logLevelArguments, new 
Maps.EntryTransformer() {
+  @Override
+  public String transformEntry(String loggerName, LogEntry.Level 
level) {
+return level.name();
+  }
+});
+  }
+
+  /**
+   * Convert the log level argument type to LogEntry.Level
+   */
+  public static Map 
convertLogLevelValuesToLogEntry(Map logLevelArguments) {
+return Maps.transformEntries(logLevelArguments, new 
Maps.EntryTransformer() {
+  @Override
+  public LogEntry.Level transformEntry(String loggerName, String 
level) {
+return LogEntry.Level.valueOf(level);
+  }
+});
+  }
+
+  /**
+   * Get the log level arguments for a twill runnable.
+   *
+   * @param runnableName name of the runnable.
+   * @param logLevelArguments the arguments for all runnables.
+   * @return the map of the log level arguments for the runnable, empty if 
there is no argument.
+   */
+  public static Map getLogLevelForRunnable(
--- End diff --

Same here.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82099064
  
--- Diff: 
twill-core/src/main/java/org/apache/twill/internal/TwillRuntimeSpecification.java
 ---
@@ -72,7 +76,7 @@ public String getZkConnectStr() {
 return zkConnectStr;
   }
 
-  public RunId getTwillRunId() {
+  public RunId getTwillAppRunId() {
--- End diff --

There is only one `RunId`, why not just call if `runId` and `getRunId` for 
the field name and method name respectively? 


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82098461
  
--- Diff: 
twill-common/src/main/java/org/apache/twill/internal/Constants.java ---
@@ -82,6 +82,14 @@ private Files() {
 }
   }
 
+  /**
+   * Constants for logLevel system messages.
+   */
+  public static final class SystemMessages {
--- End diff --

Why need string constants here? All system messages/commands should be 
defined in `SystemMessages`. You can use object comparison instead of string 
comparison.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82098250
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillPreparer.java 
---
@@ -236,6 +236,23 @@
   TwillPreparer setLogLevel(LogEntry.Level logLevel);
 
   /**
+   * Set the log levels for requested logger names for Twill applications 
running in a container.
+   *
+   * @param logLevelAppArgs The {@link Map} contains the requested logger 
names and log levels that need to be set.
+   * @return This {@link TwillPreparer}.
+   */
+  TwillPreparer setLogLevel(Map logLevelAppArgs);
--- End diff --

`setLogLevels`.


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82097429
  
--- Diff: twill-api/src/main/java/org/apache/twill/api/TwillController.java 
---
@@ -89,4 +90,29 @@
* @return A {@link Future} that will be completed when the restart 
operation has been done.
*/
   Future restartInstances(String runnable, int instanceId, int... 
moreInstanceIds);
+
+  /**
+   * Set the root log level for Twill applications running in a container.
--- End diff --

In a container? Which container? Isn't that this affect all containers of 
all runnables as well as the AM?


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

[GitHub] twill pull request #14: TWILL-138 Runtime log level change

[chtyim]

Github user chtyim commented on a diff in the pull request:

https://github.com/apache/twill/pull/14#discussion_r82097947
  
--- Diff: 
twill-api/src/main/java/org/apache/twill/api/TwillRunResources.java ---
@@ -59,6 +61,16 @@
   /**
* @return the enabled log level for the container where the runnable is 
running in.
*/
-  LogEntry.Level getLogLevel();
+  LogEntry.Level getRootLogLevel();
+
+  /**
+   * @return the enabled log level arguments for the container where the 
runnable is running in.
+   */
+  Map getLogLevelArguments();
--- End diff --

What do mean by "LogLevelArguments"? Should be "getLogLevels".


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