Skip to content

STORM-1129: Update ui to use topology name - #1277

Closed
priyank5485 wants to merge 1 commit into
apache:1.x-branchfrom
priyank5485:STORM-1129-1.x
Closed

STORM-1129: Update ui to use topology name#1277
priyank5485 wants to merge 1 commit into
apache:1.x-branchfrom
priyank5485:STORM-1129-1.x

Conversation

@priyank5485

@priyank5485priyank5485 commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

No description provided.

@priyank5485priyank5485 mentioned this pull request Mar 30, 2016
Closed
@abhishekagarwal87

abhishekagarwal87 commented Mar 30, 2016

Copy link
Copy Markdown
Contributor

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

abhishekagarwal87
abhishekagarwal87 reviewed Mar 30, 2016
View reviewed changes
Comment threadstorm-core/src/ui/public/component.html Outdated
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+'/'+

@abhishekagarwal87abhishekagarwal87Mar 30, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be left as it is.

@priyank5485

priyank5485 commented Mar 30, 2016

Copy link
Copy Markdown
ContributorAuthor

@abhishekagarwal87 Thanks for the catch. I have updated the PR. Can you please review?

knusbaum
knusbaum reviewed Mar 30, 2016
View reviewed changes
Comment threadstorm-core/src/clj/org/apache/storm/ui/core.clj
:else nil)
_ (when (empty? matching-topologies) (throw (NotAliveException. (str topology-name-or-id " is not alive"))))
id ((first matching-topologies) "id")]
id))

@knusbaumknusbaumMar 30, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@priyank5485priyank5485Mar 30, 2016

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@knusbaumknusbaumMar 30, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine with me.

@abhishekagarwal87

abhishekagarwal87 commented Mar 31, 2016

Copy link
Copy Markdown
Contributor

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

@priyank5485

priyank5485 commented Mar 31, 2016

Copy link
Copy Markdown
ContributorAuthor

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

@abhishekagarwal87

abhishekagarwal87 commented Mar 31, 2016

Copy link
Copy Markdown
Contributor

I think either way is fine. My only concern was logviewer links which I think you have addressed.

@harshach

harshach commented Jul 10, 2016

Copy link
Copy Markdown
Contributor

@priyank5485 sorry for the delay on this. Can you up merge this patch.

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Derek Dagit
Closes Pull Requests unchanged in 2018
fcda81c
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
Derek Dagit
Closes Pull Requests unchanged in 2018
fdb95d0
We are closing stale Pull Requests to make the list more manageable.
Please re-open any Pull Request that has been closed in error.
Closesapache#608Closesapache#639Closesapache#640Closesapache#648Closesapache#662Closesapache#668Closesapache#692Closesapache#705Closesapache#724Closesapache#728Closesapache#730Closesapache#753Closesapache#803Closesapache#854Closesapache#922Closesapache#986Closesapache#992Closesapache#1019Closesapache#1040Closesapache#1041Closesapache#1043Closesapache#1046Closesapache#1051Closesapache#1078Closesapache#1146Closesapache#1164Closesapache#1165Closesapache#1178Closesapache#1213Closesapache#1225Closesapache#1258Closesapache#1259Closesapache#1268Closesapache#1272Closesapache#1277Closesapache#1278Closesapache#1288Closesapache#1296Closesapache#1328Closesapache#1342Closesapache#1353Closesapache#1370Closesapache#1376Closesapache#1391Closesapache#1395Closesapache#1399Closesapache#1406Closesapache#1410Closesapache#1422Closesapache#1427Closesapache#1443Closesapache#1462Closesapache#1468Closesapache#1483Closesapache#1506Closesapache#1509Closesapache#1515Closesapache#1520Closesapache#1521Closesapache#1525Closesapache#1527Closesapache#1544Closesapache#1550Closesapache#1566Closesapache#1569Closesapache#1570Closesapache#1575Closesapache#1580Closesapache#1584Closesapache#1591Closesapache#1600Closesapache#1611Closesapache#1613Closesapache#1639Closesapache#1703Closesapache#1711Closesapache#1719Closesapache#1737Closesapache#1760Closesapache#1767Closesapache#1768Closesapache#1785Closesapache#1799Closesapache#1822Closesapache#1824Closesapache#1844Closesapache#1874Closesapache#1918Closesapache#1928Closesapache#1937Closesapache#1942Closesapache#1951Closesapache#1957Closesapache#1963Closesapache#1964Closesapache#1965Closesapache#1967Closesapache#1968Closesapache#1971Closesapache#1985Closesapache#1986Closesapache#1998Closesapache#2031Closesapache#2032Closesapache#2071Closesapache#2076Closesapache#2108Closesapache#2119Closesapache#2128Closesapache#2142Closesapache#2174Closesapache#2206Closesapache#2297Closesapache#2322Closesapache#2332Closesapache#2341Closesapache#2377Closesapache#2414Closesapache#2469
@d2rd2r mentioned this pull request Oct 16, 2018
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@priyank5485@abhishekagarwal87@harshach@knusbaum