You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by caofangkun <gi...@git.apache.org> on 2015/01/27 09:18:45 UTC
[GitHub] storm pull request: STORM-638:UI should show up process-id of the ...
GitHub user caofangkun opened a pull request:
https://github.com/apache/storm/pull/396
STORM-638:UI should show up process-id of the Worker to which an Executor is assigned
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/caofangkun/apache-storm storm-638
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/396.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 #396
----
commit 6c04e1bea802d28ae92a234ddbaad2c1ed0a6835
Author: caofangkun <ca...@gmail.com>
Date: 2015-01-23T01:28:46Z
Merge pull request #4 from apache/master
Merger from apache/storm to caofangkun/apache-storm
commit 6eedd8dccd2a3405d68de611c5da0645678645ac
Author: caofangkun <ca...@gmail.com>
Date: 2015-01-27T08:13:26Z
STORM-638:UI should show up process-id of the Worker to which an Executor is assigned
----
---
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-638:UI should show up process-id of the ...
Posted by vesense <gi...@git.apache.org>.
Github user vesense commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-83920789
unnecessary
---
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-638:UI should show up process-id of the ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-82397663
@harshach, If you are running the UI without a filter I would not consider that secure. There is other more damaging information exposed on the UI already. Also what is having the pid of the process going to let me do as an attacher? If I cannot log onto the box the pid is really rather useless. If I can log onto the box I can already get the pid by running ```ps -aef | grep '<topology-id>-<port-number>'```. Granted I am not a malicious individual so perhaps I am missing something or perhaps an attach will show up in the future that is somehow related to the pid. It is totally possible, but I don't see it as being that likely.
@caofangkun, The generated thrift code no longer compiles. It looks like you regenerated with thrift7, and we have moved on to thrift 0.9.2. Please regenerate the code again.
---
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-638:UI should show up process-id of the ...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-148204828
-0
I don't really think this is a useful thing to add to the UI. If we want to add it to the JSON API, I'm okay with that, but let's not display it on the UI.
---
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-638:UI should show up process-id of the ...
Posted by caofangkun <gi...@git.apache.org>.
Github user caofangkun commented on a diff in the pull request:
https://github.com/apache/storm/pull/396#discussion_r26362269
--- Diff: storm-core/src/storm.thrift ---
@@ -205,7 +205,8 @@ struct ExecutorSummary {
2: required string component_id;
3: required string host;
4: required i32 port;
- 5: required i32 uptime_secs;
+ 5: required i32 process_id;
--- End diff --
Fixed. Thank you. Please review this again.
---
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-638:UI should show up process-id of the ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-148209473
Agreed on @knusbaum.
It can reduce one ps command for actual users to operate (ps, kill, etc.) specific worker by hand, but I'm curious that there're other benefits.
I also think that since we're already defining "port", we would be better to encapsulate worker process into "port" and provide a way to operate such things via "port".
Or we can think about providing workers' information (usage of CPU, MEM, etc. but except secure things) into UI. Maybe we could provide a way to kill specific worker from UI, but I wonder there's such use cases.
---
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-638:UI should show up process-id of the ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/396#discussion_r26242766
--- Diff: storm-core/src/ui/public/templates/component-page-template.html ---
@@ -211,6 +216,7 @@
<td>{{uptime}}</td>
<td>{{host}}</td>
<td><a href="{{workerLogLink}}">{{port}}</a></td>
+ <td>{{processId}}
--- End diff --
Can we close the td tag? I know it is not required but all the other ones are closed.
---
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-638:UI should show up process-id of the ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/396
---
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-638:UI should show up process-id of the ...
Posted by knusbaum <gi...@git.apache.org>.
Github user knusbaum commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-193489321
Closing 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: STORM-638:UI should show up process-id of the ...
Posted by caofangkun <gi...@git.apache.org>.
Github user caofangkun commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-71607591
Pay Attention: conflicts with https://github.com/apache/storm/pull/296
---
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-638:UI should show up process-id of the ...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-78354003
@revans2 not quite comfortable with exposing pid on UI. UI is supposed to be available for all the users. In non-secure env it can be run without any filter so I am against exposing pid on the UI.
---
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-638:UI should show up process-id of the ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-82399938
Sorry it is not the code generation after all. Although the comment in the generated code with a 0.7.0 is off putting, and I would prefer it to be generated with 0.9.2. by making the field optional it changes the constructor, so now backtype/storm/daemon/nimbus.clj:1325:47 is failing to compile, because the constructor is different.
---
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-638:UI should show up process-id of the ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/396#issuecomment-78343992
I'm not convinced having the PID on the UI is really that helpful, but it does provide more information for possible automation or monitoring, so I am OK with 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.
---
[GitHub] storm pull request: STORM-638:UI should show up process-id of the ...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/396#discussion_r26242669
--- Diff: storm-core/src/storm.thrift ---
@@ -205,7 +205,8 @@ struct ExecutorSummary {
2: required string component_id;
3: required string host;
4: required i32 port;
- 5: required i32 uptime_secs;
+ 5: required i32 process_id;
--- End diff --
You cannot change a thrift ID like this. It breaks backwards compatibility too much. Please make sure that you add new tags instead of changing existing ones. It is also usually preferable to make the new tag optional, so you can have forward and backwards compatibility.
---
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.
---