You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by erikdw <gi...@git.apache.org> on 2015/09/14 22:52:47 UTC
[GitHub] storm pull request: configurable supervisor log filename
GitHub user erikdw opened a pull request:
https://github.com/apache/storm/pull/733
configurable supervisor log filename
For the storm-on-mesos project there are multiple Storm Supervisors on
every worker host. Each of these Supervisors is dedicated to a
particular topology. A challenge with this design is that the shared
supervisor log file contains logs from every single topology that
has run on a particular host. We instead want the option to have
separate supervisor log files for each topology, so that topology owners
can more easily debug their own problems.
This change allows having configurable log files for every launch of
the Supervisor, since we will be able to embed the topology ID into the
supervisor log filename.
Notably, storm-0.10 already includes a similar change for the worker
log filenames (came in with Yahoo!'s multi-tenant changes).
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/erikdw/storm configurable-supervisor-log-filename
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/733.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 #733
----
commit 38139303f6777e978516e9d4ad884220d9aad38b
Author: Erik Weathers <er...@gmail.com>
Date: 2015-09-14T20:46:36Z
configurable supervisor log filename
For the storm-on-mesos project there are multiple Storm Supervisors on
every worker host. Each of these Supervisors is dedicated to a
particular topology. A challenge with this design is that the shared
supervisor log file contains logs from every single topology that
has run on a particular host. We instead want the option to have
separate supervisor log files for each topology, so that topology owners
can more easily debug their own problems.
This change allows having configurable log files for every launch of
the Supervisor, since we will be able to embed the topology ID into the
supervisor log filename.
Notably, storm-0.10 already includes a similar change for the worker
log filenames (came in with Yahoo!'s multi-tenant changes).
----
---
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: configurable supervisor log filename
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/733
---
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: configurable supervisor log filename
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-142772155
Thanks @revans2 for giving your opinion.
@erikdw I backported your PR to 0.9.x-branch now.
Thanks for great work!
---
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: configurable supervisor log filename
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141545439
Thanks guys! Can I help with backporting it to the 0.9.6 train? I sent a PR to that branch, but I feel like that might been counterproductive? Also, is there any chance of getting this into 0.10? Not sure how the release processes are handled, but happy to become more knowledgeable and involved.
---
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: configurable supervisor log filename
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141549873
+1 on merging into 0.10
---
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: configurable supervisor log filename
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-142127363
@erikdw
Storm basically doesn't consider about multiple supervisors in one machine, I think, unless Storm includes support for YARN and Mesos.
So intermixing logs is not violating project's goal, but I also agree it's not elegant as playing with YARN, Mesos, etc.
I still think we should block new features or improvements to get into 0.9.x version line since last feature release was 10 months ago, and only two maintenance releases were published.
But I'm also thinking that we need to support storm-on-mesos, and patch is really small so we may merge this in 0.9.6 ```exceptionally```.
I'd like to hear any PMCs opinions about this before merging this into 0.9.x. I'd be fine if one of PMCs agree to include this to 0.9.x.
Could you post it to dev. mailing list?
---
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: configurable supervisor log filename
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141532910
+1. sorry for the delay.
---
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: configurable supervisor log filename
Posted by Parth-Brahmbhatt <gi...@git.apache.org>.
Github user Parth-Brahmbhatt commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141532980
will wait for 24 hours as per our Bylaws and merge it in tomorrow.
---
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: configurable supervisor log filename
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-142288795
I personally am fine with merging this into 0.9.6. It seems like a small enough change and is very well defined.
---
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: configurable supervisor log filename
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-142105000
@HeartSaVioR : thanks for the review merge!
As for the target release: I'm not sure if there's a black-and-white definition of what a "bug" constitutes for Apache Storm. i.e., I would probably consider this problem a "bug" since without this fix the logging is basically "buggy" for storm-on-mesos, since it intermixes topologies' supervisor logs, thus violating the project's goal of keeping topologies as separate as possible. I'd really appreciate it if we could be flexible on this issue and get the fix back into 0.9.x train so that it's immediately usable by our project when 0.9.6 ships, as opposed to requiring people to upgrade to 0.10.0 (a much bigger upgrade, with way more potential to have issues).
---
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: configurable supervisor log filename
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-140591396
@harshach : maybe you are willing trade me reviewing this PR for my [fixes to SECURITY.md](https://github.com/apache/storm/pull/735)? :smiling_imp:
I also have a PR of this change going into the storm 0.9.x-branch: https://github.com/apache/storm/pull/738
(Not sure if that's the proper process, haven't found any docs on how the various version branches are handled)
---
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: configurable supervisor log filename
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141547493
@erikdw This is a small enough change that I personally would feel OK with pulling this into the 0.10 release branch, but I would want at least one other committer to also agree.
---
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: configurable supervisor log filename
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141844927
+1 on merging into master and 0.10.0.
Btw, I'd like to see a JIRA issue so that we can leave this feature into CHANGELOG.
---
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: configurable supervisor log filename
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141541329
+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] storm pull request: configurable supervisor log filename
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-142135005
@HeartSaVioR : sure, shall send an email to the dev list, thank you for your thoughtful consideration.
To be clear, I didn't mean to imply that this "bug" was violating the *Storm* project's goals, just was saying that it was violating one of the goal's of the storm-on-mesos project.
---
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: configurable supervisor log filename
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-142454751
Thanks @revans2 ! @HeartSaVioR : please let me know if there's anything I can do to help with the merge.
---
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: configurable supervisor log filename
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141990307
@erikdw
Since it is not a bug, I don't think we should merge this into 0.9.x-branch.
0.9.x versions will be phased out, and we encourage users to use new version for playing with new 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 pull request: configurable supervisor log filename
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141854862
@HeartSaVioR : here ya go: https://issues.apache.org/jira/browse/STORM-1056
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 pull request: configurable supervisor log filename
Posted by erikdw <gi...@git.apache.org>.
Github user erikdw commented on the pull request:
https://github.com/apache/storm/pull/733#issuecomment-141532453
@revans2 & @ptgoetz I know y'all are busy, but is there anything I can do to get this minor (yah, "minor", I know, right?) change reviewed? The Mesos Apache project has a "shepherd" process, is there something similar for Storm?
---
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.
---