Common log directory patch

[pdread100]

This will allow the current logviewer implementation work with the UI by creating the worker logs in a common log directory. The default will be /tmp/mesos/storm/logs, see topology.worker.log.dir in storm.yaml.

I would suggest either the current multiple supervisor feature be replaced with a single supervisor per host similar to how storm works. Or multiple logviewers be launched as mesos tasks with dynamic ports and then make the UI mesos aware (like nimbus) so it gets it logviewer port information from mesos.

[erikdw]

@pdread100 : thanks for working on this!

The previous version was already using bin/storm, but you've now added it into this repo -- why is that? i.e., why was it "ok" before but not now? Note that I'm yet to have time to figure out how "regular" deploying with this framework occurs. (We copy the java bits into our own separate maven project internally right now, I'm looking to get on the more normal deploy process, whatever that is.)

Oh, and regarding your suggestions, here are my responses:

1. "the current multiple supervisor feature be replaced with a single supervisor per host similar to how storm works" -> this has a potentially negative effect depending on what you want from the framework: the individual storm topologies will no longer be isolated from one another. That feature of the current framework is actually the main reason my company is using storm-on-mesos, so it would be an unacceptable loss of functionality for what I still perceive as very little gain (the logviewer is useless if you are operating at scale).
2. The complications with any potential implementation of multiple logviewers is detailed in my comment on the earlier logviewer PR: Added logviewer startup from supervisor #38 (comment)
   • Seems like a lot of complication for, again, little gain.

So, given my reluctance to pursue a perhaps "cleaner" solution than single logviewer per host, can you tell me what your concerns with the single logviewer are? Perhaps just that we are spilling logs outside of the container's work dir, so we are imposing a requirement of log management outside of mesos on the slave hosts?

[pdread100]

@erikdw The reason I added bin/storm is also the reason I don't like this "patch". To get the common dir to take effect I had to redefine how the logback/cluster.xml was defined by isolating the Metrics and Access logs so they would stay in their individual directories, then define storm.log.dir to be the new common logging dir since this is the only way to effect the logging for the workers. The side effect was I had to add hooks into the storm script to override the storm.log.dir and the supervisor.log to have the executor ID added since the supervisor log used the storm.log.dir as well.

However this could complicate upgrading to newer versions of storm if the bin/storm script changes and or the logback/cluster.xml changes. I don't really like this approach but I was pressed to get a fix out and move on to other tasks. There may be a cleaner way of doing this but I did not see it. The biggest problem is how the storm clj scripts really don't give many options in how to define the log locations. It would have been easier if they had used WORKER_LOG_DIR as a third option to launch a worker instead of using the same storm.log.dir for both supervisor and worker.

[erikdw]

@pdread100 : I agree that it would be nice if Storm had more flexible logging in general. Specifically I wanna separate the supervisor logs & worker logs on a per-topology basis, instead of having the supervisor logs shared and the worker logs only based on port (which become shared over time as topology workers come and go).

So if I'm understanding you correctly, the extra effort of copying and changing the storm-core stuff was for separating the worker logs to the external dir, and keeping the supervisor logs in the mesos executor's work_dir. If that's the case, then maybe you can instead avoid these changes and just allow the supervisor logs to go to the same external place as the worker logs?

[pdread100]

@erikdw The definition of storm.log.dir gets set in bin/storm and of course is used to direct the supervisor.log. Supervisor then uses this definition when it launches a worker. That was the first change to bin/storm, to allow the setting of storm.log.dir so it gets passed through the supervisor to the worker. This had the side effect of placing the supervisor.log in the same location as the worker logs which was a problem since there were multiple supervisor.logs. So the second change to bin/storm was to accept the executor ID and append it to the name of the supervisor.log to make them distinct. Both worker logs and supervisor logs are placed in the common log directory and since both use the same property, storm.log.dir, I did not see much choice.

I consider this a hack but storm was not designed to run under other frameworks. It would probably be a good idea if the mesos/storm developers would nudge the storm design where possible to accommodate running under mesos.

[erikdw]

@pdread100 : hey Paul, just wanted to acknowledge your response. I need to spend more time groking and hacking and then get back to you.

[erikdw]

@pdread100 : (I've been way too busy!) So I've just been implementing a feature for my company to separate the logs per-topology in storm 0.9.4. The storm worker logs are separable via storm-core clojure changes -- it's already done in storm 0.10.0 (which is still in beta), so I just backported the required changes. But for the supervisor logs we need something like what you've done. So I think we should try to get the SUPERVISOR_LOG env variable added to bin/storm in storm proper. That's the only change you made right? Also, if we do this kind of total-copying of code we must note the specific commit we pulled the source file from. Ideally with 2 separate commits, 1 that is just the original file in its unmodified form, and the 2nd which is the changes we are making to the file.

[pdread100]

@erikdw Sounds like storm is moving in our direction. There is a STORM_SUPERVISOR_LOG which if set is applied to the SUPERVISOR_LOG in the bin/storm script. There was also a STORM_DIR which may not be necessary now that they have split the workerlogs. I would have to play with the new changes but thats not going to happen soon. I have been placed on two new tasks and the storm log is very low priority. So I doubt there will be much movement on this.

[brndnmtthws]

I'm not sure I like this approach, to be honest. I think the long-term solution is to fix Storm itself to permit dynamic ports for the logviewer (like it does for workers). Thoughts?

[erikdw]

@brndnmtthws : sure, I agree that what you just mentioned (separate logviewers with dynamic ports that are tied to the topologies on the hosts) is the best long-term solution. But I'm a bit worried about push-back from storm, since from their perspective it might be weird to handle this mesos-specific case of having topology-specific logviewers on each host. I'll file a ticket and reference our various efforts here. Or look and see if there was already a ticket, I forget off the top of my head. It would be something that our team can work on actually, so maybe there is hope!

[brndnmtthws]

@erikdw It's a legit concern, but I think if the patch was reasonable, the Storm team would be happy to accept it.

[erikdw]

@brndnmtthws & @pdread100 : I've filed a ticket against the Apache Storm project to add support for multiple container-specific logviewers: STORM-1342 Will require discussions with the Storm PMC type people to try to come to an acceptable design, not planning on working on that in the next couple of months.

Per our discussions above, and this new ticket in STORM, I'm going to close out this PR.