[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2150 +1. @harshach @priyank5485 can you please take one final look. If you don't have any objection, I suggest that we merge this patch in the next day or so. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2150 @hmcl Sorry, I accidentally overwrote this PR with the STORM-2548 content. This shouldn't contain the STORM-2548 changes. I added the getter and fixed the content error. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2150 @srdo Thanks for your diligence and awesome work refactoring this code. It just made it much better. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2150 @srdo I am +1 on this PR. Let's just add the getter method as we agreed on [this comment](https://github.com/apache/storm/pull/2150#discussion_r124855414), and have this PR consist of two commits only. The commit in [STORM-2548 PR](https://github.com/apache/storm/pull/2155) and its own commit. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user hmcl commented on the issue: https://github.com/apache/storm/pull/2150 +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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2150 The issues raised in https://issues.apache.org/jira/browse/STORM-2600 aren't really related to this PR, since the pattern based subscriptions were already unsupported by storm-kafka-monitor prior to this change. I think we should look at those issues separately. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2150 @priyank5485 Thanks for responding, I replied on the linked issue --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user priyank5485 commented on the issue: https://github.com/apache/storm/pull/2150 @srdo @harshach Some comments related to storm-kafka-monitor [~Srdo] Regarding first point, because the lag request is a http pull request from UI, as long as kafkaSpoutConfig.getSubscription().getTopicsString(); returns the correct value it will work since the open method would have been called eventually. The only change we would need is that when the rebalance listener is invoked we would need to keep track of the topics currently subscribed. For e.g. PatternSubscription can have an instance variable called Set topics that would be correctly updated anytime onPartitionsRevoked or onPartitionsAssigned is called. We can use that instance variable to return the value when getTopicsString is called on that object. Does that work? Regarding point 2, we could move the kafka client version to storm-kafka-monitor module. Can you elaborate a bit more on your concern? Is it that if kafka cluster is upgraded, storm-kafka-monitor wont work? In that case storm-kafka-client module will also have to be updated and topology jar rebuilt. Correct? I think in general, we have a certain compatibility restriction in that a given storm release works or has been tested with a certain version of kafka clients. Correct me if i am wrong. Regarding 3, the main reason for creating a separate module and calling bash script from storm UI is so that storm-core does not have a direct kafka dependency since that made sense. For windows, cygwin can be a workaround. Plus, i dont know how many windows users we have. We can start a thread to see if there is really a user base for which we need to support a use case. I dont know the details about how metrics would work. We could have opinions from other people. If that is indeed the right way to go about it then I am all for changing 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] storm issue #2150: STORM-2541: Fix storm-kafka-client manual subscription no...
Github user srdo commented on the issue: https://github.com/apache/storm/pull/2150 Created the issue for storm-kafka-monitor here https://issues.apache.org/jira/browse/STORM-2600 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
