You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by zhuoliu <gi...@git.apache.org> on 2015/10/27 15:55:27 UTC
[GitHub] storm pull request: [YSTORM-901] Worker Artifacts Directory for Hi...
GitHub user zhuoliu opened a pull request:
https://github.com/apache/storm/pull/822
[YSTORM-901] Worker Artifacts Directory for Hierarchical Log Management and Debugging
Originally all logs (both for workers and daemons) are under storm-home/logs, this causes name confusion ("wc22-1-1445957152-worker-6700.log") and difficulty for debugging (hard to store and manage heapdump and gc log files).
For better management, we present the Worker Artifacts Directory for Hierarchical Log Management and Debugging:
1. workers' logs and metadata are moved to the new
directory "storm-local/workers-artifacts", while daemon logs are still placed in storm-home/logs.
2. Cleanup, view and list implementations are modified accordingly in logviewer and UI. Specifically, we add per-worker quota and total quota for files' size, and the cleanup thread will periodically cleanup the oldest files in each worker directory and the whole artifacts if the quota is exceeded. Note that, the active workers' active logs will be skipped during cleaning.
3. In addition, a symlink called artifacts is created
from each worker's directory to its log directory "workers-artifacts/topoId/port/".
gc.log and coredump files can be configured by users to place under this symlink, thus able to be downloaded from UI conveniently.
4. Unit tests are added and the event log are supported under each worker's artifacts directory.
Current log structures:
--topoId0
--topoId1
------port1
------port2
---------worker.log
---------worker.log.err
---------worker.log.out
---------worker.yaml
---------gc.log
----------heapdump
----------events.log
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zhuoliu/storm 901
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/822.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 #822
----
commit 4ab6a0f2d2af36102157b632ee55929534165b36
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-26T16:37:43Z
STORM-901 worker-artifacts for logviewer
commit 28899baf3a24ab435cbb3db13decafb652ba3afe
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-26T20:50:10Z
Solve event-log issue, ui, dir, routes, listLogs
commit 8d9bb1fb8cddef900d53d42b06fd8952cb5b4cb7
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-27T02:59:27Z
Fix supervisor test
commit c7bbd091b06b68bd8d22a6f66a4210bcf4a475fa
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-27T14:34:03Z
Add unit tests for logviewer cleanup, filter, sorting, etc.
----
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/822#discussion_r43155803
--- Diff: storm-core/src/jvm/backtype/storm/utils/Utils.java ---
@@ -692,5 +696,65 @@ public static void handleUncaughtException(Throwable t) {
}
}
}
+
+ /**
+ * Given a File input it will unzip the file in a the unzip directory
+ * passed as the second parameter
+ * @param inFile The zip file as input
+ * @param unzipDir The unzip directory where to unzip the zip file.
+ * @throws IOException
+ */
+ public static void unZip(File inFile, File unzipDir) throws IOException {
+ Enumeration<? extends ZipEntry> entries;
+ ZipFile zipFile = new ZipFile(inFile);
+
+ try {
+ entries = zipFile.entries();
+ while (entries.hasMoreElements()) {
+ ZipEntry entry = entries.nextElement();
+ if (!entry.isDirectory()) {
+ InputStream in = zipFile.getInputStream(entry);
+ try {
+ File file = new File(unzipDir, entry.getName());
+ if (!file.getParentFile().mkdirs()) {
+ if (!file.getParentFile().isDirectory()) {
+ throw new IOException("Mkdirs failed to create " +
+ file.getParentFile().toString());
+ }
+ }
+ OutputStream out = new FileOutputStream(file);
+ try {
+ byte[] buffer = new byte[8192];
+ int i;
+ while ((i = in.read(buffer)) != -1) {
+ out.write(buffer, 0, i);
+ }
+ } finally {
+ out.close();
+ }
+ } finally {
+ in.close();
+ }
+ }
+ }
+ } finally {
+ zipFile.close();
+ }
+ }
+
+ //Note: Only works for zip files whose uncompressed size is less than 4 GB
+ //Otherwise returns the size module 2^32, per gzip specifications
+ //Returns a long, since that's what file lengths in Java/Clojure usually are.
--- End diff --
Can we turn this into a javadoc comment?
---
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-901] Worker Artifacts Directory for Hie...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/822
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163781066
@harshach sorry I misread your comment.
Moving them to be under storm-log-dir by default would be fine. I would want a config to put it some other place if we want to, but the default could be under the log directory.
@zhuoliu could you file a JIRA for this? it should be just a few lines of change to try and read a new config with a default being the value of `storm.log.dir`
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163492846
On the contrary, say a supervisor has been running for a long period (say a week), 24 workers * 10 restarting* 40 files per worker (10 worker.log.gz files, 10 gc log files, error files, out files and many other files). We will have around 10000 files from all topologies' workers and all daemons in the single storm-home directory, with tedious long file names. This may not be an ideal status.
---
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-901] Worker Artifacts Directory for Hie...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/822#discussion_r43398781
--- Diff: storm-core/src/clj/backtype/storm/daemon/logviewer.clj ---
@@ -294,9 +418,55 @@ Note that if anything goes wrong, this will throw an Error and exit."
(resp/status 404))
(unauthorized-user-html user))))
+(defn daemonlog-page [fname start length grep user root-dir]
+ (if (or (blank? (*STORM-CONF* UI-FILTER))
+ (authorized-log-user? user fname *STORM-CONF*)) ;; how to deal with this???????????
+ (let [file (.getCanonicalFile (File. root-dir fname))
+ file-length (.length file)
+ path (.getCanonicalPath file)
+ zip-file? (.endsWith path ".gz")]
+ (if (and (= (.getCanonicalFile (File. root-dir))
+ (.getParentFile file))
+ (.exists file))
+ (let [file-length (if zip-file? (Utils/zipFileSize (clojure.java.io/file path)) (.length (clojure.java.io/file path)))
+ length (if length
+ (min 10485760 length)
+ default-bytes-per-page)
+ log-files (into [] (filter #(.isFile %) (.listFiles (File. root-dir)))) ;all types of files included
+ files-str (for [file log-files]
+ (.getName file))
+ reordered-files-str (conj (filter #(not= fname %) files-str) fname)
+ is-txt-file (re-find #"\.(log.*|txt|yaml|pid)$" fname)
--- End diff --
We might have pulled this out into its own function, since it is used at least twice now. Not a big deal though.
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-151535592
Reopen and make sure it connects with JIRA
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163713836
@harshach and @erikdw can you explain the exact use cases you are concerned about? From previous interactions it feels like you are concerned about storm on slider on YARN and storm on mesos respectively. So the fix is to place the logs in a configurable place that other tools can get access to?
I just want to be sure I understand the use case so I can properly address those concerns. We can move the worker-artifacts dir to be in a configurable location. That should be totally doable. I wouldn't really want them all to be under storm.log.dir, as @zhuoliu pointed out there are more than just logs stored here. As part of this change we essentially upgraded the logs to be application metadata that can have limits placed on them and managed. But if you really want it based off of storm.log.dir as a default it is not a deal breaker.
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/822#discussion_r43155407
--- Diff: storm-core/src/clj/backtype/storm/util.clj ---
@@ -580,6 +582,21 @@
(when-not success?
(throw (RuntimeException. (str "Failed to touch " path))))))
+(defn create-symlink!
+ "Create symlink is to the target"
+ ([path-dir target-dir file-name]
+ (create-symlink! path-dir target-dir file-name file-name))
+ ([path-dir target-dir from-file-name to-file-name]
+ (let [path (str path-dir file-path-separator from-file-name)
+ target (str target-dir file-path-separator to-file-name)
+ empty-array (make-array String 0)
+ attrs (make-array FileAttribute 0)
+ abs-path (.toAbsolutePath (Paths/get path empty-array))
+ abs-target (.toAbsolutePath (Paths/get target empty-array))]
+ (log-message "Creating symlink [" abs-path "] to [" abs-target "]")
--- End diff --
This should probably be debug if we keep it around at all.
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163780366
It would be a rather large change to support two different directory structures, one for logs and one for everything else. It is not impossible to do, but it would add quite a bit of complexity to something that is already more complex then I like.
I guess it comes down to how often do you and your customers access the logs through the storm UI or some other automated log system vs how often you access the logs by going to the box directly and reading them directly. For us our clusters have grown large enough that very few people try to read the logs directly. Finding the right node is such a pain that the added directly structure is not something they have complained about.
Would having the worker-artifacts directory structure by default be under `storm.log.dir` but also have a separate config be an acceptable solution? This is a very simple change to make, vs trying to split the data.
---
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-901] Worker Artifacts Directory for Hie...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/822#discussion_r43398696
--- Diff: storm-core/src/clj/backtype/storm/daemon/logviewer.clj ---
@@ -294,9 +418,55 @@ Note that if anything goes wrong, this will throw an Error and exit."
(resp/status 404))
(unauthorized-user-html user))))
+(defn daemonlog-page [fname start length grep user root-dir]
+ (if (or (blank? (*STORM-CONF* UI-FILTER))
+ (authorized-log-user? user fname *STORM-CONF*)) ;; how to deal with this???????????
--- End diff --
I think we can remove this comment.
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
GitHub user zhuoliu reopened a pull request:
https://github.com/apache/storm/pull/822
[STORM-901] Worker Artifacts Directory for Hierarchical Log Management and Debugging
Originally all logs (both for workers and daemons) are under storm-home/logs, this causes name confusion ("wc22-1-1445957152-worker-6700.log") and difficulty for debugging (hard to store and manage heapdump and gc log files for all topologies' workers in a single directory).
For better management, we present the Worker Artifacts Directory for Hierarchical Log Management and Debugging:
1. workers' logs and metadata are moved to the new directory "storm-local/workers-artifacts", while daemon logs are still placed in storm-home/logs.
2. Cleanup, view and list implementations are modified accordingly in logviewer and UI. Specifically, we add per-worker quota and total quota for files' size, and the cleanup thread will periodically cleanup the oldest files in each worker directory and the whole artifacts if the quota is exceeded. Note that, the active workers' active logs will be skipped during cleaning.
3. In addition, a symlink called artifacts is created from each worker's directory to its log directory "workers-artifacts/topoId/port/". gc.log and heapdump files can be configured by users to place under this symlink, thus able to be downloaded from UI conveniently. Other useful features can be easily support by worker artifacts directory, where their resource files can be dumped.
4. Unit tests are added and the event log are supported under each worker's artifacts directory.
Current log structures under storm-local/workers-artifacts:
--topoId0
--topoId1
------port1
------port2
---------worker.log
---------worker.log.err
---------worker.log.out
---------worker.yaml
---------gc.log
----------heapdump
----------events.log
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zhuoliu/storm 901
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/822.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 #822
----
commit 4ab6a0f2d2af36102157b632ee55929534165b36
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-26T16:37:43Z
STORM-901 worker-artifacts for logviewer
commit 28899baf3a24ab435cbb3db13decafb652ba3afe
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-26T20:50:10Z
Solve event-log issue, ui, dir, routes, listLogs
commit 8d9bb1fb8cddef900d53d42b06fd8952cb5b4cb7
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-27T02:59:27Z
Fix supervisor test
commit c7bbd091b06b68bd8d22a6f66a4210bcf4a475fa
Author: zhuol <zh...@yahoo-inc.com>
Date: 2015-10-27T14:34:03Z
Add unit tests for logviewer cleanup, filter, sorting, etc.
----
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-151583919
Oh and with adding in support for nimbus.log, we need to be careful that the code does not allow for files outside of the logs or the worker artifacts directories to be downloaded.
---
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-901] Worker Artifacts Directory for Hie...
Posted by unsleepy22 <gi...@git.apache.org>.
Github user unsleepy22 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163489832
I agree with @harshach , it's a big breaking change of log layout, as @arunmahadevan says, maybe it's better to make a symlink?
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163490768
That is doable though weird, but I would be interested to see why we want all logs to be found under a single directory.
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-151580821
@zhuoliu all of the tests are failing with
```
java.nio.file.NoSuchFileException: /tmp/667af8d4-e62c-4bee-a874-118c019c478d/workers-artifacts/test-1-1445964515/1027/events.log
at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86) ~[?:1.8.0_31]
at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102) ~[?:1.8.0_31]
at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107) ~[?:1.8.0_31]
at sun.nio.fs.UnixFileSystemProvider.newByteChannel(UnixFileSystemProvider.java:214) ~[?:1.8.0_31]
at java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:434) ~[?:1.8.0_31]
at java.nio.file.Files.newOutputStream(Files.java:216) ~[?:1.8.0_31]
at java.nio.file.Files.newBufferedWriter(Files.java:2860) ~[?:1.8.0_31]
at backtype.storm.metric.FileBasedEventLogger.initLogWriter(FileBasedEventLogger.java:32) [classes/:?]
at backtype.storm.metric.FileBasedEventLogger.prepare(FileBasedEventLogger.java:85) [classes/:?]
at backtype.storm.metric.EventLoggerBolt.prepare(EventLoggerBolt.java:39) [classes/:?]
at backtype.storm.daemon.executor$fn__7917$fn__7930.invoke(executor.clj:819) [classes/:?]
at backtype.storm.util$async_loop$fn__1152.invoke(util.clj:480) [classes/:?]
at clojure.lang.AFn.run(AFn.java:22) [clojure-1.7.0.jar:?]
at java.lang.Thread.run(Thread.java:745) [?:1.8.0_31]
```
It looks like the event logger test is trying to write to a directory that does not exist.
---
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-901] Worker Artifacts Directory for Hie...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/822#discussion_r43399109
--- Diff: storm-core/src/clj/backtype/storm/daemon/logviewer.clj ---
@@ -333,42 +503,112 @@ Note that if anything goes wrong, this will throw an Error and exit."
(name k) "'")
ex)))))
+(defn list-log-files
+ [user topoId port log-root callback origin]
+ (let [file-results
+ (if (nil? topoId)
+ (if (nil? port)
+ (get-all-logs-for-rootdir (File. log-root))
+ (reduce concat
+ (for [topo-dir (.listFiles (File. log-root))]
+ (reduce concat
+ (for [port-dir (.listFiles topo-dir)]
+ (if (= (str port) (.getName port-dir))
+ (into [] (.listFiles port-dir))))))))
+ (if (nil? port)
+ (let [topo-dir (File. (str log-root file-path-separator topoId))]
+ (if (.exists topo-dir)
+ (reduce concat
+ (for [port-dir (.listFiles topo-dir)]
+ (into [] (.listFiles port-dir))))
+ []))
+ (let [port-dir (get-worker-dir-from-root log-root topoId port)]
+ (if (.exists port-dir)
+ (into [] (.listFiles port-dir))
+ []))))
+ file-strs (sort (for [file file-results]
+ (get-topo-port-workerlog file)))]
+ (json-response file-strs
+ callback
+ :headers {"Access-Control-Allow-Origin" origin
+ "Access-Control-Allow-Credentials" "true"})))
+
(defroutes log-routes
(GET "/log" [:as req & m]
- (try
- (let [servlet-request (:servlet-request req)
- log-root (:log-root req)
- user (.getUserName http-creds-handler servlet-request)
- start (if (:start m) (parse-long-from-map m :start))
- length (if (:length m) (parse-long-from-map m :length))]
- (log-template (log-page (:file m) start length (:grep m) user log-root)
- (:file m) user))
- (catch InvalidRequestException ex
- (log-error ex)
- (ring-response-from-exception ex))))
+ (try
+ (let [servlet-request (:servlet-request req)
+ log-root (:log-root req)
+ user (.getUserName http-creds-handler servlet-request)
+ start (if (:start m) (parse-long-from-map m :start))
+ length (if (:length m) (parse-long-from-map m :length))
+ file (url-decode (:file m))]
+ (log-template (log-page file start length (:grep m) user log-root)
+ file user))
+ (catch InvalidRequestException ex
+ (log-error ex)
+ (ring-response-from-exception ex))))
+ (GET "/daemonlog" [:as req & m]
+ (try
+ (let [servlet-request (:servlet-request req)
+ daemonlog-root (:daemonlog-root req)
+ user (.getUserName http-creds-handler servlet-request)
+ start (if (:start m) (parse-long-from-map m :start))
+ length (if (:length m) (parse-long-from-map m :length))
+ file (url-decode (:file m))]
+ (log-template (daemonlog-page file start length (:grep m) user daemonlog-root)
+ file user))
+ (catch InvalidRequestException ex
+ (log-error ex)
+ (ring-response-from-exception ex))))
(GET "/download/:file" [:as {:keys [servlet-request servlet-response log-root]} file & m]
- (try
- (let [user (.getUserName http-creds-handler servlet-request)]
- (download-log-file file servlet-request servlet-response user log-root))
- (catch InvalidRequestException ex
- (log-error ex)
- (ring-response-from-exception ex))))
+ ;; We do not use servlet-response here, but do not remove it from the
+ ;; :keys list, or this rule could stop working when an authentication
+ ;; filter is configured.
+ (try
+ (let [user (.getUserName http-creds-handler servlet-request)]
+ (download-log-file file servlet-request servlet-response user log-root))
+ (catch InvalidRequestException ex
+ (log-error ex)
+ (ring-response-from-exception ex))))
+ (GET "/daemondownload/:file" [:as {:keys [servlet-request servlet-response daemonlog-root]} file & m]
+ ;; We do not use servlet-response here, but do not remove it from the
+ ;; :keys list, or this rule could stop working when an authentication
+ ;; filter is configured.
--- End diff --
Same comment; remove this text?
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-151582912
The log link on the UI for the nimbus.log is not working. We probably should do something to special case the daemon log files.
---
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-901] Worker Artifacts Directory for Hie...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163669693
@harshach
Use case:
A cluster admin shelling to a supervisor node and browsing all current logs easily, without having to `cd` to other directories?
Are there other use cases? What makes this unacceptable to users? Maybe we can come up with something better in general.
> We can let the supervisor to redirect(create) the worker-port.log symlink to the topo/port/worker.log in launch-worker. Thanks for the explain. Btw, we should care that some symlink may become dead link since its pointed file might have been deleted by cleanup.
I am not excited about doing this, because it adds complexity to storm that ought to be handled by either a logging system or shell environment. It seems to me to make more sense to provide a tool for shells to make browsing logs easier where they are, rather than adding complexity to storm to support the shell.
---
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-901] Worker Artifacts Directory for Hie...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163489194
@zhuoliu additionally can we create symlinks from storm-home/logs/worker-port.log -> workers-artifacts/topo/port/worker.log so that all the recent worker logs can be found under a single dir ?
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163488283
Hi @harshach , the main reasons we change log location are two-fold: 1) we want to separate the daemon logs from worker logs for better namespace and permission management; 2) the workers-artifacts directory are beyond "just logs", it appears more like a drop-box for each worker, containing log files, heapdump, gc files, and other possible files users may need to dump.
It will be one line change to "config/worker-artifacts-root" function if we want to place worker-artifacts under storm-home/logs/. In that way it will look as follows. In our opinion, it would be preferable to let workers-artifacts to stay with storm-local/workers, worker-users.
We can discuss which one is more appropriate.
storm-home/logs/
------supervisor.log
------nimbus.log
------workers-artifacts/
-----------topo1/
------------------port1/
------------------port2/
------------------------worker.log
------------------------worker.log.err
------------------------gc.log
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163783382
sure. this conclusion makes sense to me and won't be much change. A JIRA is filed here STORM-1387
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163744508
@erikdw Thanks for the feedback, yes the changes should be something that you can do through `storm.yaml`.
@harshach do you have any feedback?
---
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-901] Worker Artifacts Directory for Hie...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163493600
@zhuoliu i meant only the current worker-port.log file, which would make it a bit easier to switch between files while tailing. Right now we need to change the entire path to switch to a different worker log file. The rotated and log.gz files can be under workers-artifacts.
---
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-901] Worker Artifacts Directory for Hie...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163697172
@arunmahadevan @d2r @zhuoliu As Arun said above if the current worker-port.log is in root dir it will be helpful to tail the logs. This preferable but not necessary. My main concern is we are not using storm.log.dir and adding logs to storm-local dir which is supposed to be used to store storm metadata dir and other topology metadata not application logs. And changing log location of topologies while using storm.log.dir is not a desired behavior. If we can store all the logs in user configured storm.log.dir than I am fine with current structure.
---
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-901] Worker Artifacts Directory for Hie...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-152214222
Just some minor comments.
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-152220780
Looks good I am +1 once the comments from @d2r are 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-901] Worker Artifacts Directory for Hie...
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163700356
+1 to @harshach 's comment. We rely on being able to modify the `storm.log.dir` location to change where logs go.
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu closed the pull request at:
https://github.com/apache/storm/pull/822
---
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-901] Worker Artifacts Directory for Hie...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-152393655
+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-901] Worker Artifacts Directory for Hie...
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163721659
@revans2 : good memory, my use-case is indeed storm-on-mesos. And in the way my company deploys it, we leverage the `storm.log.dir` to force the logs to be written into a known location where we can forward them to various downstream log-indexing/storing services. That is the use case we want to be supported *somehow*. I'm not particularly married to `storm.log.dir`, so your proposal to have the `worker-artifacts` go to some other location seems sufficient for my use case. I'll explicitly state that I would expect it to be a knob that is configurable from within `storm.yaml`.
BTW, changing to a hierarchical layout from a flat style was concerning at first blush, but on further reflection I believe it will work just fine. Our tools for log-forwarding can use globs/wildcards so it should be 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: [STORM-901] Worker Artifacts Directory for Hie...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/storm/pull/822#discussion_r43399062
--- Diff: storm-core/src/clj/backtype/storm/daemon/logviewer.clj ---
@@ -333,42 +503,112 @@ Note that if anything goes wrong, this will throw an Error and exit."
(name k) "'")
ex)))))
+(defn list-log-files
+ [user topoId port log-root callback origin]
+ (let [file-results
+ (if (nil? topoId)
+ (if (nil? port)
+ (get-all-logs-for-rootdir (File. log-root))
+ (reduce concat
+ (for [topo-dir (.listFiles (File. log-root))]
+ (reduce concat
+ (for [port-dir (.listFiles topo-dir)]
+ (if (= (str port) (.getName port-dir))
+ (into [] (.listFiles port-dir))))))))
+ (if (nil? port)
+ (let [topo-dir (File. (str log-root file-path-separator topoId))]
+ (if (.exists topo-dir)
+ (reduce concat
+ (for [port-dir (.listFiles topo-dir)]
+ (into [] (.listFiles port-dir))))
+ []))
+ (let [port-dir (get-worker-dir-from-root log-root topoId port)]
+ (if (.exists port-dir)
+ (into [] (.listFiles port-dir))
+ []))))
+ file-strs (sort (for [file file-results]
+ (get-topo-port-workerlog file)))]
+ (json-response file-strs
+ callback
+ :headers {"Access-Control-Allow-Origin" origin
+ "Access-Control-Allow-Credentials" "true"})))
+
(defroutes log-routes
(GET "/log" [:as req & m]
- (try
- (let [servlet-request (:servlet-request req)
- log-root (:log-root req)
- user (.getUserName http-creds-handler servlet-request)
- start (if (:start m) (parse-long-from-map m :start))
- length (if (:length m) (parse-long-from-map m :length))]
- (log-template (log-page (:file m) start length (:grep m) user log-root)
- (:file m) user))
- (catch InvalidRequestException ex
- (log-error ex)
- (ring-response-from-exception ex))))
+ (try
+ (let [servlet-request (:servlet-request req)
+ log-root (:log-root req)
+ user (.getUserName http-creds-handler servlet-request)
+ start (if (:start m) (parse-long-from-map m :start))
+ length (if (:length m) (parse-long-from-map m :length))
+ file (url-decode (:file m))]
+ (log-template (log-page file start length (:grep m) user log-root)
+ file user))
+ (catch InvalidRequestException ex
+ (log-error ex)
+ (ring-response-from-exception ex))))
+ (GET "/daemonlog" [:as req & m]
+ (try
+ (let [servlet-request (:servlet-request req)
+ daemonlog-root (:daemonlog-root req)
+ user (.getUserName http-creds-handler servlet-request)
+ start (if (:start m) (parse-long-from-map m :start))
+ length (if (:length m) (parse-long-from-map m :length))
+ file (url-decode (:file m))]
+ (log-template (daemonlog-page file start length (:grep m) user daemonlog-root)
+ file user))
+ (catch InvalidRequestException ex
+ (log-error ex)
+ (ring-response-from-exception ex))))
(GET "/download/:file" [:as {:keys [servlet-request servlet-response log-root]} file & m]
- (try
- (let [user (.getUserName http-creds-handler servlet-request)]
- (download-log-file file servlet-request servlet-response user log-root))
- (catch InvalidRequestException ex
- (log-error ex)
- (ring-response-from-exception ex))))
+ ;; We do not use servlet-response here, but do not remove it from the
+ ;; :keys list, or this rule could stop working when an authentication
+ ;; filter is configured.
--- End diff --
It seems we do use `servlet-response` here, so can we remove this comment?
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-152293023
Really appreciate the review efforts. @d2r 's comments have been addressed, welcome to check back.
---
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-901] Worker Artifacts Directory for Hie...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/822#discussion_r43154970
--- Diff: storm-core/src/clj/backtype/storm/daemon/logviewer.clj ---
@@ -219,7 +321,7 @@ Note that if anything goes wrong, this will throw an Error and exit."
(if (and appender-name appender (instance? RollingFileAppender appender))
(.getParent (File. (.getFileName appender)))
(throw
- (RuntimeException. "Log viewer could not find configured appender, or the appender is not a FileAppender. Please check that the appender name configured in storm and log4j2 agree.")))))
+ (RuntimeException. "Log viewer could not find configured appender, or the appender is not a FileAppender. Please check that the appender name configured in storm and logback agree.")))))
--- End diff --
This exception should be log4j2 not logback
---
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-901] Worker Artifacts Directory for Hie...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163776916
@revans2 I want user experience to be consistent . I.e when they configure storm.log.dir , the logs to be appear in configured storm.log.dir , not some of the logs in storm.log.dir and others in storm-local/worker-artifacts. Its hard to understand why they are ending up there. In some cases storm-local dir might only accessed by linux user "storm" who is running the daemons. My request is to keep the current hierarchical dir that we've right now but move all the logs to configured storm.log.dir not have it in storm-local/worker-artifacts. Not sure if this requires lot of changes for this patch.
---
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-901] Worker Artifacts Directory for Hie...
Posted by unsleepy22 <gi...@git.apache.org>.
Github user unsleepy22 commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163813319
@revans2 @zhuoliu This solution looks good to me too.
My initial concern was sort of like @erikdw 's use case. And usually files in logs directory do a rolling update/delete(maybe by a cron job), while we may not want a rolling delete for metadata(or at least different delete stragety), so it's good to add an extra config in storm.yaml to specify this directory.
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163494981
@arunmahadevan , if we do have this application scenario, it would be nice to do that. We can let the supervisor to redirect(create) the worker-port.log symlink to the topo/port/worker.log in launcher-worker.
---
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-901] Worker Artifacts Directory for Hie...
Posted by zhuoliu <gi...@git.apache.org>.
Github user zhuoliu commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-151635950
Thanks @revans2.
New web routes are added for supporting view, list and download of all daemon logs.
The concern of directory access permission have been addressed also (through verification of the root log dir).
---
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-901] Worker Artifacts Directory for Hie...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/822#issuecomment-163486612
@zhuoliu @revans2 @d2r Hi, @arunmahadevan pointed out this jira changed log location for worker logs. This is actually not acceptable for our users and I don't see any reason for log location to change since we've storm.log.dir and its been used in previous releases . If its necessary for this patch to function to have the logs in storm-local/worker-artifacts than we should put in storm.log.dir and symlink to storm-local/worker-artifacts. Checking to see the reason behind moving the logs there before sending a patch. Thanks.
---
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.
---