You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by priyank5485 <gi...@git.apache.org> on 2016/03/30 06:43:19 UTC
[GitHub] storm pull request: STORM-1129: Update ui to use topology name
GitHub user priyank5485 opened a pull request:
https://github.com/apache/storm/pull/1277
STORM-1129: Update ui to use topology name
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/priyank5485/storm STORM-1129-1.x
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1277.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 #1277
----
commit 2648d74cad7d1e46ec9bcceb9b3b388cb98cfc8d
Author: Priyank <ps...@hortonworks.com>
Date: 2016-03-29T05:31:08Z
STORM-1129: Update ui to use topology 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #1277: STORM-1129: Update ui to use topology name
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/1277
@priyank5485 sorry for the delay on this. Can you up merge 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-1129: Update ui to use topology name
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1277#issuecomment-203756449
I think either way is fine. My only concern was logviewer links which I think you have 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-1129: Update ui to use topology name
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1277#discussion_r57951884
--- Diff: storm-core/src/clj/org/apache/storm/ui/core.clj ---
@@ -466,6 +466,21 @@
"assignedCpu" (.get_assigned_cpu t)})
"schedulerDisplayResource" (*STORM-CONF* Config/SCHEDULER_DISPLAY_RESOURCE)}))
+(defn get-topology-id [topology-name-or-id]
+ (let [summary ((all-topologies-summary) "topologies")
+ filter-fun-name (fn[topo-summary] (= (topo-summary "name") topology-name-or-id))
+ filter-fun-id (fn[topo-summary] (= (topo-summary "id") topology-name-or-id))
+ matching-topologies-by-name (filter filter-fun-name summary)
+ matching-topologies-by-id (filter filter-fun-id summary)
+ matching-topologies
+ (cond
+ (not-empty matching-topologies-by-name) matching-topologies-by-name
+ (not-empty matching-topologies-by-id) matching-topologies-by-id
+ :else nil)
+ _ (when (empty? matching-topologies) (throw (NotAliveException. (str topology-name-or-id " is not alive"))))
+ id ((first matching-topologies) "id")]
+ id))
--- End diff --
That's fine with me.
---
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-1129: Update ui to use topology name
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1277#issuecomment-203279789
@priyank5485 I see that you have the changed the logviewer urls in the topology page to use topology name but there are no changes in logviewer.clj. I think logviewer links should just point to topology id since log links are anyway tied to a specific instance of topology run.
---
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-1129: Update ui to use topology name
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/1277#discussion_r57948138
--- Diff: storm-core/src/clj/org/apache/storm/ui/core.clj ---
@@ -466,6 +466,21 @@
"assignedCpu" (.get_assigned_cpu t)})
"schedulerDisplayResource" (*STORM-CONF* Config/SCHEDULER_DISPLAY_RESOURCE)}))
+(defn get-topology-id [topology-name-or-id]
+ (let [summary ((all-topologies-summary) "topologies")
+ filter-fun-name (fn[topo-summary] (= (topo-summary "name") topology-name-or-id))
+ filter-fun-id (fn[topo-summary] (= (topo-summary "id") topology-name-or-id))
+ matching-topologies-by-name (filter filter-fun-name summary)
+ matching-topologies-by-id (filter filter-fun-id summary)
+ matching-topologies
+ (cond
+ (not-empty matching-topologies-by-name) matching-topologies-by-name
+ (not-empty matching-topologies-by-id) matching-topologies-by-id
+ :else nil)
+ _ (when (empty? matching-topologies) (throw (NotAliveException. (str topology-name-or-id " is not alive"))))
+ id ((first matching-topologies) "id")]
+ id))
--- End diff --
We should probably prioritize matching ids here instead of matching names. As it is now, If I give my topology a name that matches another topology's id, the topology whose id I stole will become inaccessible. Since we're prioritizing names, it will match that topology's id to my topology's name, and display my topology 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-1129: Update ui to use topology name
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on the pull request:
https://github.com/apache/storm/pull/1277#issuecomment-203743167
Thanks @priyank5485 - Though I am still not sure how logviewer links will work. topologyId variable is not assigned any value. Original code -
https://github.com/apache/storm/blob/master/storm-core/src/ui/public/component.html#L136
---
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-1129: Update ui to use topology name
Posted by priyank5485 <gi...@git.apache.org>.
Github user priyank5485 commented on the pull request:
https://github.com/apache/storm/pull/1277#issuecomment-203557013
@abhishekagarwal87 Thanks for the catch. I have updated the PR. Can you please review?
---
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 #1277: STORM-1129: Update ui to use topology name
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1277
---
[GitHub] storm pull request: STORM-1129: Update ui to use topology name
Posted by abhishekagarwal87 <gi...@git.apache.org>.
Github user abhishekagarwal87 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1277#discussion_r57843883
--- Diff: storm-core/src/ui/public/component.html ---
@@ -176,7 +176,7 @@
var loc = $(row[3])[0]; // logviewer URL
return '<input type="checkbox" class="workerActionCheckbox"'+
'id="'+checkboxId+'" value="'+host_port+'"'+checkedString+'/> '+
- '<a href="'+loc.protocol+'//'+loc.host+'/dumps/'+topologyId+'/'+
+ '<a href="'+loc.protocol+'//'+loc.host+'/dumps/'+topologyName+'/'+
--- End diff --
should be left as it is.
---
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-1129: Update ui to use topology name
Posted by priyank5485 <gi...@git.apache.org>.
Github user priyank5485 commented on a diff in the pull request:
https://github.com/apache/storm/pull/1277#discussion_r57950485
--- Diff: storm-core/src/clj/org/apache/storm/ui/core.clj ---
@@ -466,6 +466,21 @@
"assignedCpu" (.get_assigned_cpu t)})
"schedulerDisplayResource" (*STORM-CONF* Config/SCHEDULER_DISPLAY_RESOURCE)}))
+(defn get-topology-id [topology-name-or-id]
+ (let [summary ((all-topologies-summary) "topologies")
+ filter-fun-name (fn[topo-summary] (= (topo-summary "name") topology-name-or-id))
+ filter-fun-id (fn[topo-summary] (= (topo-summary "id") topology-name-or-id))
+ matching-topologies-by-name (filter filter-fun-name summary)
+ matching-topologies-by-id (filter filter-fun-id summary)
+ matching-topologies
+ (cond
+ (not-empty matching-topologies-by-name) matching-topologies-by-name
+ (not-empty matching-topologies-by-id) matching-topologies-by-id
+ :else nil)
+ _ (when (empty? matching-topologies) (throw (NotAliveException. (str topology-name-or-id " is not alive"))))
+ id ((first matching-topologies) "id")]
+ id))
--- End diff --
@knusbaum Thats true. But if we prioritize ids would not your topology become inaccessible? I think going forward we should use topology names in ui and elsewhere so that we can have permanent links and since thats the identifier given by the user. Support for lookup with ids was added for backwards compatibility as suggested by @revans2 and others.
---
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-1129: Update ui to use topology name
Posted by priyank5485 <gi...@git.apache.org>.
Github user priyank5485 commented on the pull request:
https://github.com/apache/storm/pull/1277#issuecomment-203751947
@abhishekagarwal87 Please look at the $.getJson method. Line 190 in component.html. Now, instead of topologyId being assigned on onReady i do it in $.getJson. Its a side effect of squashing commits that makes it hard to look at the commits that addressed review comments. Sorry about that. Do you guys usually keep separate commits and squash in the end? I will do that next time.
---
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.
---