Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/644#issuecomment-130315702
For me internally the tests pass and everything builds. Although the rest
of storm seems a bit unstable with tests.
I think the changes look fine +1 for merging this in.
I don't think we have any split brain problems because of how we route
messages, so the begin command and commit messages should prevent this.
There are some things that I think we can do better, but are not wrong.
1) I am scared that the transaction file is going to put more of a load on
the Name Node than I like. I am mostly concerned that if we have thousands of
instances of hdfs state creating/deleting transaction files multiple times a
second the load is going to noticeable on a large cluster. But this is a
premature optimization so I would say we don't address it unless we see it as
an actual problem.
2) The RotationActions don't have a recovery interface, and things like the
MoveFile action could potentially fail after successfully rotating the file
which might leave the file in the wrong directory.
3) I don't really like the special case for the TimedRotationAction.start.
I would rather see start a part of the RotationAction interface, that can be
ignored by those that don't want/need it.
The last two could perhaps be a follow on JIRA. I am fine with the code as
is, but I would like another pair of eyes looking at this too before merging 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 [email protected] or file a JIRA ticket
with INFRA.
---
