Github user ogorun commented on the pull request:
https://github.com/apache/storm/pull/387#issuecomment-72505185
Hi @Parth-Brahmbhatt ,
Thank you for pointing me to this pull request. I see that my pull request
https://github.com/apache/storm/pull/406 shares part of ideas with your branch
Storm 631 and think it would be nice to join the ideas in one place.
The main point of intersection is isolating failure-related functionality
in dedicated class.
* The difference in my approach is that I see this class plugable since
logic of failure handling can differ from topology to topology. For example, I
implemented case when number of retries per message is restricted. Of course,
reasonable default should be provided. If it is pluggable, the question is how
to choose and configure it. I think two main possibilities are: either with
extending of KafkaConfig class, or introducing registry that stores appropriate
factory. Last approach can be better in case we speak about choosing approach
per topology that works with several KafkaSpouts, listening to different
topics.
* One more (minor) point. I would move all fail() event handling to failure
handler. Your implementation divides this logic to common part located in
PartitionManager class and specific one - in failure handler. I don't see
conceptual cause for this division. There is a technical issue related to
access level of members in PartitionManager, but this might be changed by
slight PartitionManager refactoring.
I'd be glad to see your feedback to these points
---
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.
---
