You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by kishorvpatil <gi...@git.apache.org> on 2015/09/28 23:00:59 UTC
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
GitHub user kishorvpatil opened a pull request:
https://github.com/apache/storm/pull/766
[STORM-412] Allow users to modify logging levels of running topologies
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kishorvpatil/incubator-storm STORM-412
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/766.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #766
----
commit d13cfe2dc9beda8fc4fda6fa6536d87b9ce514a1
Author: Kishor Patil <kp...@yahoo-inc.com>
Date: 2015-09-24T14:22:37Z
Adding thrift changes for Dynamic Logging
commit 250ab1132e5cb26ac9cf1af5c47cbaf98390923a
Author: Kishor Patil <kp...@yahoo-inc.com>
Date: 2015-09-25T20:00:45Z
Adding Dynamic Logger to UI, cli and supervisors
commit 087f08ea314df2055bce61abbb54cc7443da41a9
Author: Alessandro Bellina <ab...@yahoo-inc.com>
Date: 2015-04-10T17:07:22Z
Documentation brief for dynamic log level settings feature
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-145252754
I am still +1 on this. @kishorvpatil The merge conflict is very minor in the commands list in storm.py, please upmerge.
@Parth-Brahmbhatt I really would like to get this in soon. Please take a look at the fix so I can pull this in.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/766#discussion_r40736026
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -518,6 +540,13 @@
(catch KeeperException e
(log-warn-error e "Could not teardown errors for " storm-id))))
+ (teardown-topology-log-config!
--- End diff --
I saw that you are calling it from cleanup thread, I am just wondering why not call cleanup with all the other topology cleanup methods?
From what I understand the nimbus cleanup thread is designed to identify topologies that have no nimbus local disk state but their zookeeper state is somehow not cleared up. It seems like a handler for rare failure scenarios which we are abusing now. Let me know if you have some other reason for not deleting this with rest of the topology.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/766#discussion_r40676193
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -518,6 +540,13 @@
(catch KeeperException e
(log-warn-error e "Could not teardown errors for " storm-id))))
+ (teardown-topology-log-config!
--- End diff --
@Parth-Brahmbhatt. Currently, I am calling, the `teardown-topology-log-config!` is called from within `do-cleanup ` method on `nimbus.clj`.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/766#discussion_r40671792
--- Diff: storm-core/src/clj/backtype/storm/command/set_log_level.clj ---
@@ -0,0 +1,75 @@
+;; 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.
+(ns backtype.storm.command.set-log-level
+ (:use [clojure.tools.cli :only [cli]])
+ (:use [backtype.storm thrift log])
+ (:import [org.apache.logging.log4j Level])
+ (:import [backtype.storm.generated LogConfig LogLevel LogLevelAction])
+ (:gen-class))
+
+(defn- get-storm-id
+ "Get topology id for a running topology from the topology name."
+ [nimbus name]
+ (let [info (.getClusterInfo nimbus)
+ topologies (.get_topologies info)
+ topology (first (filter (fn [topo] (= name (.get_name topo))) topologies))]
+ (if topology
+ (.get_id topology)
+ (throw (.IllegalArgumentException (str name " is not a running topology"))))))
+
+(defn- parse-named-log-levels [action]
+ "Parses [logger name]=[level string]:[optional timeout],[logger name2]...
+
+ e.g. ROOT=DEBUG:30
+ root logger, debug for 30 seconds
+
+ org.apache.foo=WARN
+ org.apache.foo set to WARN indefinitely"
+ (fn [^String s]
+ (let [log-args (re-find #"(.*)=([A-Z]+):?(\d*)" s)
+ name (if (= action LogLevelAction/REMOVE) s (nth log-args 1))
+ level (Level/toLevel (nth log-args 2))
+ timeout-str (nth log-args 3)
+ log-level (LogLevel.)]
+ (if (= action LogLevelAction/REMOVE)
+ (.set_action log-level action)
+ (do
+ (.set_action log-level action)
+ (.set_target_log_level log-level (.toString level))
+ (.set_reset_log_level_timeout_secs log-level
+ (Integer. (if (= timeout-str "") "0" timeout-str)))))
+ {name log-level})))
+
+(defn- merge-together [previous key val]
+ (assoc previous key
+ (if-let [oldval (get previous key)]
+ (merge oldval val)
+ val)))
+
+(defn -main [& args]
+ (let [[{log-setting :log-setting remove-log-setting :remove-log-setting} [name] _]
+ (cli args ["-l" "--log-setting"
+ :parse-fn (parse-named-log-levels LogLevelAction/UPDATE)
+ :assoc-fn merge-together]
+ ["-r" "--remove-log-setting"
+ :parse-fn (parse-named-log-levels LogLevelAction/REMOVE)
+ :assoc-fn merge-together])]
+ (with-configured-nimbus-connection nimbus
--- End diff --
It might be nice to move this down to just around line 75. That way we don't establish a connection to nimbus even when there are errors, but honestly this is very very minor, and I am OK without 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/766#discussion_r40678621
--- Diff: storm-core/src/clj/backtype/storm/command/set_log_level.clj ---
@@ -0,0 +1,75 @@
+;; 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.
+(ns backtype.storm.command.set-log-level
+ (:use [clojure.tools.cli :only [cli]])
+ (:use [backtype.storm thrift log])
+ (:import [org.apache.logging.log4j Level])
+ (:import [backtype.storm.generated LogConfig LogLevel LogLevelAction])
+ (:gen-class))
+
+(defn- get-storm-id
+ "Get topology id for a running topology from the topology name."
+ [nimbus name]
+ (let [info (.getClusterInfo nimbus)
+ topologies (.get_topologies info)
+ topology (first (filter (fn [topo] (= name (.get_name topo))) topologies))]
+ (if topology
+ (.get_id topology)
+ (throw (.IllegalArgumentException (str name " is not a running topology"))))))
+
+(defn- parse-named-log-levels [action]
+ "Parses [logger name]=[level string]:[optional timeout],[logger name2]...
+
+ e.g. ROOT=DEBUG:30
+ root logger, debug for 30 seconds
+
+ org.apache.foo=WARN
+ org.apache.foo set to WARN indefinitely"
+ (fn [^String s]
+ (let [log-args (re-find #"(.*)=([A-Z]+):?(\d*)" s)
+ name (if (= action LogLevelAction/REMOVE) s (nth log-args 1))
+ level (Level/toLevel (nth log-args 2))
+ timeout-str (nth log-args 3)
+ log-level (LogLevel.)]
+ (if (= action LogLevelAction/REMOVE)
+ (.set_action log-level action)
+ (do
+ (.set_action log-level action)
+ (.set_target_log_level log-level (.toString level))
+ (.set_reset_log_level_timeout_secs log-level
+ (Integer. (if (= timeout-str "") "0" timeout-str)))))
+ {name log-level})))
+
+(defn- merge-together [previous key val]
+ (assoc previous key
+ (if-let [oldval (get previous key)]
+ (merge oldval val)
+ val)))
+
+(defn -main [& args]
+ (let [[{log-setting :log-setting remove-log-setting :remove-log-setting} [name] _]
+ (cli args ["-l" "--log-setting"
+ :parse-fn (parse-named-log-levels LogLevelAction/UPDATE)
+ :assoc-fn merge-together]
+ ["-r" "--remove-log-setting"
+ :parse-fn (parse-named-log-levels LogLevelAction/REMOVE)
+ :assoc-fn merge-together])]
+ (with-configured-nimbus-connection nimbus
--- End diff --
Addressed.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-144197151
Tested with storm.starter.clj.word_count on a non-secured cluster. Seems to work. +1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/766
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on a diff in the pull request:
https://github.com/apache/storm/pull/766#discussion_r40617965
--- Diff: storm-core/src/clj/backtype/storm/cluster.clj ---
@@ -518,6 +540,13 @@
(catch KeeperException e
(log-warn-error e "Could not teardown errors for " storm-id))))
+ (teardown-topology-log-config!
--- End diff --
shouldn't this be called from remove-storm!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-144387766
@Parth-Brahmbhatt, thank you for pointing that out that nimbus cleanup thread is not right place for removing ZK nodes for logconfigs. I have moved the zk-node deletion to `remove-storm!`. Please let me know if you have any other suggestions.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-143872684
Thank you @abellina for all the hard work on 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-144064554
I had one minor nit, but I am +1 overall with or without the small fix, assuming you can address/answer @Parth-Brahmbhatt's comment.
The test failures look unrelated, one was a failure to bind to a given port, and the other was a kafka failure.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-145635463
Still +1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-145263234
+1.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-412] Allow users to modify logging leve...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/storm/pull/766#issuecomment-145631129
Thanks @Parth-Brahmbhatt @revans2. I upmerged against master. It is ready to merge now.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---