You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by abellina <gi...@git.apache.org> on 2016/07/26 04:21:45 UTC
[GitHub] storm pull request #1592: STORM-1994: Add table with per-topology and worker...
GitHub user abellina opened a pull request:
https://github.com/apache/storm/pull/1592
STORM-1994: Add table with per-topology and worker resource usage and\u2026
\u2026 components in (new) supervisor and topology pages
This adds a collapsible table to the topology page and a new supervisor page that show per worker assigned resources, number of executors and the components running on each worker. We use this as a debugging tool, helping identify where components are running and to visualize all running components in a specific host.
Thanks to @kishorvpatil for coming up with the idea in the first place, and reviewing and testing the feature. Also thanks to @knusbaum, and @d2r who reviewed the code and provided bug fixes and code refactors.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/abellina/storm STORM-1994_add_per_worker_components_and_resource_usage
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/1592.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 #1592
----
commit f5ad92888b6f1470e4d7f179fbe5ae73e37a07f7
Author: Alessandro Bellina <ab...@yahoo-inc.com>
Date: 2016-07-06T19:23:18Z
STORM-1994: Add table with per-topology and worker resource usage and components in (new) supervisor and topology pages
----
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by abellina <gi...@git.apache.org>.
Github user abellina commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR I should have this done over the weekend. I'll ping you when it is ready for 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 issue #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR
Good to hear it. I don't know if we want to stop feature development on 1.x since that's our release branch, and 2.x seems to have stalled. The policy behind all of this is probably better discussed on the mailing lists.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1592
@abellina No need to hurry. Just would like to hear your opinion. Please let me know once it's done.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by abellina <gi...@git.apache.org>.
Github user abellina commented on the issue:
https://github.com/apache/storm/pull/1592
Hi, I haven't gotten reviews on this ticket yet. There are a lot of generated files, so it is not as bad as it looks!
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR
I'm +1 for the patch.
Nimbus has been in progress for porting for a long time. We can't (apparently) halt all progress until it's ported. You recently submitted a patch which only modified 11 lines. If many people submit patches that only modify 11 lines, it's no different. Things seem to have been pretty consistently merged regardless of the fact that they touch un-ported clojure code.
This is also a pretty desirable code change. It provides something that we really need to provide if we want the platform to be successful. It's already helped me personally solve three or four issues that would have been extremely irritating to solve otherwise.
If we had a time-frame for Nimbus being ported, I might say "let's wait on it," but we don't. This is a really good feature. I say, let's get it 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 issue #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by abellina <gi...@git.apache.org>.
Github user abellina commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR, Are we freezing development on nimbus until it is ported? I can wait and submit when that is ported if nimbus is close, else I am not sure if it is a show stopper. Maybe I misunderstand your point.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1592
@knusbaum @jerrypeng
OK. I was just not sure that we are seeing same target version for the improvements: 1.x vs 2.x.
If we all think we would want to ship our effort to 1.x, we must not block this due to port especially given that we all don't know when the port will be finished.
(We must deal with this ASAP since it makes the contribution getting harder.)
As I said I didn't review this, but I'm +1 for this 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 issue #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1592
@jerrypeng Please let me know the issue numbers for these things. I'd also try to get some time to backport them.
@knusbaum Agreed. I recently initiated discussion to reduce the scope for porting (let clojure tests out of phase 1) and it might be not enough.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1592
Hi @abellina,
this is a bit huge patch (even without thrift generated code) including front-end change, so seems not easy for me to have time to review.
Before reviewing, two considerations from me:
1. Since recent RAS patches only applied to master branch, I'm not sure we may want to apply RAS related improvement to 1.x as well. Would like to hear opinions on @jerrypeng.
2. This changes nimbus.clj which is not ported yet, and it seems not small - more than 300 lines changed only on nimbus.clj. I recently submitted a patch which modifies nimbus.clj but it only changes 11 lines.
Btw, you said @kishorvpatil and @knusbaum, and @d2r already reviewed the patch. Then why not review this again (and leave +1)? Reviewing internally is no effect for Apache side.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by abellina <gi...@git.apache.org>.
Github user abellina commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR, thanks for taking a look and running locally. As far as the supervisor config, I added: https://issues.apache.org/jira/browse/STORM-2035. Let me know if that's what you had in mind.
As far as a patch for 1.x, I'll add a new PR. The stats code is in Java in 2.x, but clojure in 1.x.
Thanks @knusbaum, and @jerrypeng for jumping in as well.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1592
@abellina
First of all, freezing development was what we planned though it was broken now.
Let me clear on this,
- Recent patches for RAS are only applied to master (2.0.0)
- I don't know this is for freezing development on 1.x, or just forget backporting to 1.x.
- At a glimpse, this patch seems to help for RAS users.
- Could you explain what benefits this patch give to non RAS users?
- So for me this patch is more likely for 2.0.0, not 1.x.
- If this is also for 1.x, personally I'm OK to allow modification for nimbus.clj.
- But if this is just for 2.0.0, porting nimbus is same milestone so why not porting it first before adding the workload for porting?
I'm also the one who works two times for same pull request because of porting, so I might be a bit keen 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 #1592: STORM-1994: Add table with per-topology and worker...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/1592
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1592
@abellina I'd like to see how the patch for 1.x is going. If you feel we don't need to introduce this improvement at 1.x please just let me know. I'll just merge this to only master. 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.
---
[GitHub] storm issue #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by abellina <gi...@git.apache.org>.
Github user abellina commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR I pull requested the change to 1.x. Please let me know if you have questions.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by jerrypeng <gi...@git.apache.org>.
Github user jerrypeng commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR
I have pushed critical bug fixes for RAS to the 1.x branch. There has been a few algorithmic improvements on RAS that I had only put into 2.x but I desire to get them into 1.x as well when I have time.
I have found this feature to be extremely useful regardless of whether RAS is being used. It tells you what components are scheduled in each worker and tells you the uptime of each worker so you can monitor the stability of your topology.
We have no clear timeline for when the translation of nimbus will be done, and I am not sure its good to wait for an uncertain amount of time.
Thus I am +1 for this 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 issue #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by abellina <gi...@git.apache.org>.
Github user abellina commented on the issue:
https://github.com/apache/storm/pull/1592
@HeartSaVioR, the patch isn't directed for RAS specifically.
The primary benefit for either non-RAS or RAS is summarizing all workers for a topology in one table, and all workers for a supervisor in one table (with the new supervisor page). In each of these tables, each worker shows the components that are running (with the # of executors per component), uptime per worker, and resources assigned.
The supervisor page is useful because you can go and see what other topologies are running on machine X, and what components they are running. I think that view is unique.
---
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 #1592: STORM-1994: Add table with per-topology and worker resour...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/1592
Applied and ran the test with -Pall-tests passed. Deployed binary distribution and test UI. Looks good.
Assuming code review is done for other committers (two +1s).
+1 Nice work @abellina.
I would like to see configuration table as well as other page from supervisor detail page so that configuration of each machine can be easily seen, but it can be addressed to separate issue.
@abellina Could we have the patch for 1.x branch, too? Or does it be easy to apply to 1.x?
---
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.
---