You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by carl34 <gi...@git.apache.org> on 2017/05/19 20:42:37 UTC
[GitHub] storm pull request #2126: Add the option to set client.id to storm-kafka and...
GitHub user carl34 opened a pull request:
https://github.com/apache/storm/pull/2126
Add the option to set client.id to storm-kafka and storm-kafka-client
Add the ability to set the Kafka client.id property to storm-kafka and storm-kafka-client.
storm-kafka example:
```
SpoutConfig spoutConfig = new SpoutConfig(
brokers,
topicConfs.topic,
+ "client_id-" + confs.consumerGroupId + "-" + topicConfs.topic,
"/consumers",
confs.consumerGroupId + "-" + topicConfs.topic
);
```
storm-kafka-client example:
```
KafkaSpoutConfig<String,String> kafkaSpoutConfig = KafkaSpoutConfig.builder(KAFKA_LOCAL_BROKER, TOPIC)
.setGroupId(confs.consumerGroupId + "-" + topicConfs.topic)
+ .setClientId("client_id-" + confs.consumerGroupId + "-" + topicConfs.topic)
.build();
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/carl34/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2126.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2126
----
commit ed677582b20103e4f89a144895a788b64bdd94f1
Author: Carl Haferd <ch...@groupon.com>
Date: 2017-05-17T20:04:53Z
Add the option to set client.id to storm-kafka and storm-kafka-client
commit e35c3598653851b4ed6af6c5a4ea1cab3801724b
Author: carl34 <ca...@users.noreply.github.com>
Date: 2017-05-19T20:39:52Z
Merge pull request #1 from carl34/kafka_client_id
Add the option to set client.id to storm-kafka and storm-kafka-client
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by priyank5485 <gi...@git.apache.org>.
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 the method setCliendId. I would rather use setProp instead. Can you use setProp instead on the builder for now? And when i create a PR for refactoring storm-kafka-client you can comment about any issues that you might run in to because of the changes in the PR.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2126: Add the option to set client.id to storm-kafka
Posted by carl34 <gi...@git.apache.org>.
Github user carl34 closed the pull request at:
https://github.com/apache/storm/pull/2126
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by revans2 <gi...@git.apache.org>.
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 feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2126: Add the option to set client.id to storm-kafka and...
Posted by carl34 <gi...@git.apache.org>.
GitHub user carl34 reopened a pull request:
https://github.com/apache/storm/pull/2126
Add the option to set client.id to storm-kafka and storm-kafka-client
Add the ability to set the Kafka client.id property to storm-kafka and storm-kafka-client (https://issues.apache.org/jira/browse/STORM-2524).
storm-kafka example:
```
SpoutConfig spoutConfig = new SpoutConfig(
brokers,
topicConfs.topic,
+ "client_id-" + confs.consumerGroupId + "-" + topicConfs.topic,
"/consumers",
confs.consumerGroupId + "-" + topicConfs.topic
);
```
storm-kafka-client example:
```
KafkaSpoutConfig<String,String> kafkaSpoutConfig = KafkaSpoutConfig.builder(KAFKA_LOCAL_BROKER, TOPIC)
.setGroupId(confs.consumerGroupId + "-" + topicConfs.topic)
+ .setClientId("client_id-" + confs.consumerGroupId + "-" + topicConfs.topic)
.build();
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/carl34/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2126.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2126
----
commit ca50f699a4f7b50ef2f463ed076297abeabd321a
Author: Carl Haferd <ch...@groupon.com>
Date: 2017-05-17T20:04:53Z
Add the option to set client.id to storm-kafka
commit cb543c0393d23ae93c0394697d5a555037cee486
Author: carl34 <ca...@users.noreply.github.com>
Date: 2017-05-22T20:26:25Z
Merge pull request #3 from carl34/kafka_client_id
Add the option to set client.id to storm-kafka
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by carl34 <gi...@git.apache.org>.
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?
https://github.com/apache/storm/blob/a4afacd9617d620f50cf026fc599821f7ac25c79/external/storm-kafka/src/jvm/org/apache/storm/kafka/KafkaConfig.java#L32
We were initially just going to submit a PR for storm-kafka, but we're hoping to make it clear to new topology creators that client.id is available because we feel that it is valuable. If the builder pattern is going to be removed, would it be a big deal to add this change in the mean-time?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by carl34 <gi...@git.apache.org>.
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 so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2126: Add the option to set client.id to storm-kafka
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2126
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by carl34 <gi...@git.apache.org>.
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 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by priyank5485 <gi...@git.apache.org>.
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 need that method? As far as the storm-kafka module is concerned that property is already public. I dont see a reason for adding one more constructor argument.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2126: Add the option to set client.id to storm-kafka and...
Posted by carl34 <gi...@git.apache.org>.
Github user carl34 closed the pull request at:
https://github.com/apache/storm/pull/2126
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2126: Add the option to set client.id to storm-kafka and...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2126#discussion_r117774461
--- Diff: external/storm-kafka/pom.xml ---
@@ -57,7 +57,7 @@
<artifactId>maven-checkstyle-plugin</artifactId>
<!--Note - the version would be inherited-->
<configuration>
- <maxAllowedViolations>557</maxAllowedViolations>
+ <maxAllowedViolations>558</maxAllowedViolations>
--- End diff --
I would prefer to see the violations fixed instead of adding in new ones. In this case I suspect that it is missing javadocs for the new SpoutConfig constructor.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by priyank5485 <gi...@git.apache.org>.
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 constructor argument is more clear than setting a public property? If yes, I dont agree with you on that. That extra constructor does not add any value. Plus, community is encouraged to use new spout. So unless there is a blocker or critical bug in the old spout i prefer not changing any code there.
For storm-kafka-client I plan to raise a PR to get rid of the builder pattern. Builder pattern relies on builder.build() and pass the returned object to the constructor. Unfortunately, that does not work well with flux. Hence setProp method, either on the builder (or may be in future directly on KafkaSpoutConfig) is encouraged. There are so many other properties on kafka consumer api. It does not make sense to expose each one of those as a setXXX java method. Without those setXXX methods it is a much cleaner api for spout and its config object. Intellisense should not be the top priority here. I would prefer a cleaner api.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request #2126: Add the option to set client.id to storm-kafka
Posted by carl34 <gi...@git.apache.org>.
GitHub user carl34 reopened a pull request:
https://github.com/apache/storm/pull/2126
Add the option to set client.id to storm-kafka
Add the ability to set the Kafka client.id property to storm-kafka and storm-kafka-client (https://issues.apache.org/jira/browse/STORM-2524).
storm-kafka example:
```
SpoutConfig spoutConfig = new SpoutConfig(
brokers,
topicConfs.topic,
+ "client_id-" + confs.consumerGroupId + "-" + topicConfs.topic,
"/consumers",
confs.consumerGroupId + "-" + topicConfs.topic
);
```
storm-kafka-client example:
```
KafkaSpoutConfig<String,String> kafkaSpoutConfig = KafkaSpoutConfig.builder(KAFKA_LOCAL_BROKER, TOPIC)
.setGroupId(confs.consumerGroupId + "-" + topicConfs.topic)
+ .setClientId("client_id-" + confs.consumerGroupId + "-" + topicConfs.topic)
.build();
```
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/carl34/storm master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2126.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2126
----
commit 26bcfdbef95c4aecbff8aee0a587281f59b55bcf
Author: Carl Haferd <ch...@groupon.com>
Date: 2017-05-22T23:09:19Z
Add the option to set client.id to storm-kafka, STORM-2524
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka and storm-...
Posted by carl34 <gi...@git.apache.org>.
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 fetch requests contain blank "Client ID:" values. We would like to have a clear way for our developers to set client.id so that we can quickly determine the source of Kafka packets.
For storm-kafka-client, I get your point that setProp can be used, but we would like to encourage our users to set this parameter so we would appreciate if it was clearly available with the builder pattern via intellisense.
Here is an example fetch request from storm-kafka without a client.id:
```
Kafka (Fetch Request)
Length: 54
API Key: Fetch (1)
API Version: 0
Correlation ID: 0
String Length: 0
Client ID:
Replica ID: -1
Max Wait Time: 10000
Min Bytes: 0
Array Count: 1
Fetch Request Topic (1 partitions)
String Length: 6
Topic Name: canary
Array Count: 1
Fetch Request Partition (Partition-ID=2, Offset=162438425)
Partition ID: 2
Offset: 162438425
Max Bytes: 1048576
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2126: Add the option to set client.id to storm-kafka
Posted by carl34 <gi...@git.apache.org>.
Github user carl34 commented on the issue:
https://github.com/apache/storm/pull/2126
I can try rebasing again for the CHANGELOG.md updates, please let me know if I should add a new entry or to that file or if there are any other actions for me to take.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---