[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-19 Thread priyank5485
Github user priyank5485 commented on the issue: https://github.com/apache/storm/pull/2126 @carl34 We already have setProp method in KafkaSpoutConfig in storm-kafka-client that can be used to set any producer property. I dont think this adds any value. Can you elaborate on why you nee

[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-19 Thread carl34
Github user carl34 commented on the issue: https://github.com/apache/storm/pull/2126 @priyank5485, setting client.id is useful for identifying requests Kafka protocol packets. For instance, we have many topologies which run on a shared Storm cluster using storm-kafka and all of the f

[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-19 Thread priyank5485
Github user priyank5485 commented on the issue: https://github.com/apache/storm/pull/2126 @carl34 https://github.com/apache/storm/blob/master/external/storm-kafka/src/jvm/org/apache/storm/kafka/KafkaConfig.java#L32 is already a public property. Are you saying passing it as a construct

[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-19 Thread carl34
Github user carl34 commented on the issue: https://github.com/apache/storm/pull/2126 Thanks for the additional context. Our primary objective is to find a way to update our legacy topologies which use storm-kafka, but it looks like the clientId member is final in 1.1.0, correct?

[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-19 Thread priyank5485
Github user priyank5485 commented on the issue: https://github.com/apache/storm/pull/2126 @carl34 Good point. I did not realize it was final until you brought it up. In that case i am okay with your change for storm-kafka module. For storm-kafka-client module i still prefer avoiding t

[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-22 Thread revans2
Github user revans2 commented on the issue: https://github.com/apache/storm/pull/2126 Also please upmerge there is a merge conflict. --- 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 featu

[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-22 Thread carl34
Github user carl34 commented on the issue: https://github.com/apache/storm/pull/2126 Added javadocs and rebased --- 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

[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...

2017-05-22 Thread carl34
Github user carl34 commented on the issue: https://github.com/apache/storm/pull/2126 @priyank5485 sounds good, I went ahead and removed the storm-kafka-client change so that only storm-kafka is updated and we will plan to use setProp. --- If your project is set up for it, you can rep