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 2014/06/18 23:58:33 UTC
[GitHub] incubator-storm pull request: [STORM-360] Add node details for Err...
GitHub user kishorvpatil opened a pull request:
https://github.com/apache/incubator-storm/pull/150
[STORM-360] Add node details for Error Topology and Component pages
Please note that this would be not a rolling upgrade due to change in TaskError struct used to store in znode.
* Add host, port to ErrorInfo, TaskError, znode
* Added host, port to trident class ErrorInfo
* Added host, port to error znodes
* Displaying host, port and link to log file for errors on
* Topology page ( per component in spouts/bolts)
* Component page
* Fixed component template to ensure Errors section is displayed on errors.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kishorvpatil/incubator-storm node-details-on-error
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-storm/pull/150.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 #150
----
commit 550a2596f11aff07b893c24b3737c0521ffcd206
Author: Kishor Patil <kp...@yahoo-inc.com>
Date: 2014-06-18T21:43:10Z
Show node details on error
----
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14034403
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -524,7 +547,11 @@
:let [stats-seq (get-filled-stats summs)
stats (aggregate-spout-streams
(aggregate-spout-stats
- stats-seq include-sys?))]]
+ stats-seq include-sys?))
+ last-error (most-recent-error (get errors id))
+ error-host (get-error-host last-error)
+ error-port (get-error-port last-error error-host top-id)
+ ]]
--- End diff --
Please put the closing ']' on the line above.
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/150#issuecomment-46713103
Some really minor comments on the formatting, and one issue with the template. The rest looks really good.
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the pull request:
https://github.com/apache/incubator-storm/pull/150#issuecomment-46719846
@d2r @revans2 I have fixed the comments with coding style. Also fixed the issue, with templates.
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-storm/pull/150
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14037320
--- Diff: storm-core/src/ui/public/component.html ---
@@ -92,10 +96,6 @@
$("#bolt-executor-table").tablesorter({ sortList: [[0,0]], headers: {}});
}
}
- componentErrors.append(Mustache.render($(template).filter("#component-errors-template").html(),response));
- if(response["componentErrors"].length > 0) {
- $("#component-errors-table").tablesorter({ sortList: [[0,0]], headers: {1: { sorter: "stormtimestr"}}});
- }
--- End diff --
Curious, why was this 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14036506
--- Diff: storm-core/src/clj/backtype/storm/daemon/nimbus.clj ---
@@ -857,7 +857,10 @@
(defn- get-errors [storm-cluster-state storm-id component-id]
(->> (.errors storm-cluster-state storm-id component-id)
- (map #(ErrorInfo. (:error %) (:time-secs %)))))
+ (map #(let [error-info (ErrorInfo. (:error %) (:time-secs %))
+ _ (.set_host error-info (:host %))
+ _ (.set_port error-info (:port %))]
+ error-info))))
--- End diff --
Could we use doto instead?
```
(defn- get-errors [storm-cluster-state storm-id component-id]
(->> (.errors storm-cluster-state storm-id component-id)
(map #(doto (ErrorInfo. (:error %) (:time-secs %))
(.set_host (:host %))
(.set_port (:port %))))))
```
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/incubator-storm/pull/150#issuecomment-46714796
@ptgoetz, @revans2, this will break a rolling upgrade since errors have changed. Will this kind of thing require a minor version bump (e.g., 0.10.0-incubating), and should that be done with this PR or separately if so?
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14034430
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -552,7 +587,11 @@
"processLatency" (float-str (get-in stats [:process-latencies window]))
"acked" (get-in stats [:acked window])
"failed" (get-in stats [:failed window])
- "lastError" (most-recent-error (get errors id))}))
+ "errorHost" error-host
+ "errorPort" error-port
+ "errorWorkerLogLink" (worker-log-link error-host error-port)
+ "lastError" (get-error-data last-error)
+ }))
--- End diff --
Please put the closing '}))' on the line above.
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14034358
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -291,6 +290,30 @@
(map nil-to-zero)
(apply max)))
+(defn get-error-span [error]
--- End diff --
For these new functions could we follow the new convention with the parameters on the second line and put the closing ')' on the line with the last code.
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on the pull request:
https://github.com/apache/incubator-storm/pull/150#issuecomment-46737733
Manually tested to confirm errors show up on the topology and component pages, and logviewer links appear to work. Unit tests pass.
+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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14035162
--- Diff: storm-core/src/ui/public/templates/component-page-template.html ---
@@ -153,14 +154,15 @@
</tbody>
</table>
</script>
-
<script id="component-errors-template" type="text/html">
<h2>Errors</h2>
-<table class="zebra-striped" id="component-errors-table"><thead><tr><th>Time</th><th>Error</th></tr></thead>
+<table class="zebra-striped" id="component-errors-table"><thead><tr><th>Time</th><th>Error Host</th>th>Error Port</th><th>Error</th></tr></thead>
--- End diff --
Looks like you are missing a '<' for the Error Port
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/150#issuecomment-46715394
I would like us to officially support rolling upgrades from the beginning, just because we are using thrift already so it is not that hard. So I would like to see us bump the version number, but not as a part of this. It takes moving things around in JIRA a bit too.
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/incubator-storm/pull/150#issuecomment-46850093
+1 I manually tested this too.
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by d2r <gi...@git.apache.org>.
Github user d2r commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14036907
--- Diff: storm-core/src/clj/backtype/storm/ui/core.clj ---
@@ -249,9 +249,8 @@
(sort-by #(.get_error_time_secs ^ErrorInfo %))
reverse
first)]
- (if error
- (error-subset (.get_error ^ErrorInfo error))
- "")))
+ error
+ ))
--- End diff --
Closing `))` on same line
---
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] incubator-storm pull request: [STORM-360] Add node details for Err...
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/incubator-storm/pull/150#discussion_r14040080
--- Diff: storm-core/src/ui/public/component.html ---
@@ -92,10 +96,6 @@
$("#bolt-executor-table").tablesorter({ sortList: [[0,0]], headers: {}});
}
}
- componentErrors.append(Mustache.render($(template).filter("#component-errors-template").html(),response));
- if(response["componentErrors"].length > 0) {
- $("#component-errors-table").tablesorter({ sortList: [[0,0]], headers: {1: { sorter: "stormtimestr"}}});
- }
--- End diff --
Good catch. This was changed when I tried to get the component template working -unwanted. I reverted 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.
---