You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by srdo <gi...@git.apache.org> on 2017/06/05 20:01:14 UTC
[GitHub] storm pull request #2151: STORM-2542: Remove KafkaConsumer.subscribe API opt...
GitHub user srdo opened a pull request:
https://github.com/apache/storm/pull/2151
STORM-2542: Remove KafkaConsumer.subscribe API option, make KafkaConsumer.assign the default
This builds on https://github.com/apache/storm/pull/2150, which is the first commit in this. The third commit is purely class moves because the kafka.spout package was getting a bit unwieldy.
This should only go on 2.x if it goes in at all.
Please see https://issues.apache.org/jira/browse/STORM-2542 for the justification for why I believe we should make this change.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/srdo/storm STORM-2542
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2151.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 #2151
----
commit 4b4529af8ae1a519ee1eee8432ad4c628aacfe78
Author: Stig Rohde Døssing <st...@gmail.com>
Date: 2017-06-05T12:59:19Z
STORM-2541: Fix storm-kafka-client manual subscription not being able to start consuming
commit 771d0c8ffb8305a79306ab33e9e38946f7a414e9
Author: Stig Rohde Døssing <st...@gmail.com>
Date: 2017-06-05T12:59:19Z
STORM-2542: Remove storm-kafka-client KafkaConsumer.subscribe API option
commit 7032d485ec4a58a141ac5e92d69f9c748ab4b106
Author: Stig Rohde Døssing <st...@gmail.com>
Date: 2017-06-05T19:57:58Z
STORM-2542: Move some storm-kafka-client classes into their own packages
----
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2151
If this goes in I'll follow up with a PR against 1.x to deprecate the classes using `KafkaConsumer.subscribe`
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2151
@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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm issue #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2151
@srdo was this agreed upon ?
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2151
Thanks for the reviews.
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on the issue:
https://github.com/apache/storm/pull/2151
@srdo I am +1 on this PR. Let's just clean the commits that that make it such that we this PR consist of three commits only. The first commit should be [STORM-2548 PR](https://github.com/apache/storm/pull/2155), the 2nd commit [STORM-2541 PR](https://github.com/apache/storm/pull/2150), plus its own commit moving classes into the appropriate packages.
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2151
@srdo are we not planning on pushing this into 1.x-branch?
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API opt...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2151
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API opt...
Posted by hmcl <gi...@git.apache.org>.
Github user hmcl commented on a diff in the pull request:
https://github.com/apache/storm/pull/2151#discussion_r127773597
--- Diff: docs/storm-kafka-client.md ---
@@ -240,12 +240,9 @@ streams. If you are doing this for Trident a value must be in the List returned
otherwise trident can throw exceptions.
-### Manual Partition Control (ADVANCED)
+### Manual Partition Assigment (ADVANCED)
-By default Kafka will automatically assign partitions to the current set of spouts. It handles lots of things, but in some cases you may want to manually assign the partitions.
-This can cause less churn in the assignments when spouts go down and come back up, but it can result in a lot of issues if not done right. This can all be handled by subclassing
-Subscription and we have a few implementations that you can look at for examples on how to do this. ManualPartitionNamedSubscription and ManualPartitionPatternSubscription. Again
-please be careful when using these or implementing your own.
+By default the KafkaSpout instancs will be assigned partitions by round robin assignment. If you need to customize this assignment, you can implement the `ManualPartitioner` interface. The implementation can be passed to the `ManualPartitionSubscription` constructor, and the `Subscription` can then be set in the `KafkaSpoutConfig` via the `KafkaSpoutConfig.Builder` constructor. Please take care when supplying a custom implementation, since an incorrect `ManualPartitioner` implementation could leave some partitions unread, or concurrently read by multiple spout instances. See the `RoundRobinManualPartitioner` for an example of how to implement this functionality.
--- End diff --
By default the KafkaSpout instances will be assigned partitions using a round robin strategy. If you need to customize partitions assignment, you must implement the `ManualPartitioner` interface.
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2151
@hmcl No. I got no response on the mailing list, so now I'm trying here. I figure if anyone objects they'll be able to do so on this 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 issue #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on the issue:
https://github.com/apache/storm/pull/2151
+1. Thanks @srdo this looks great.
---
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 #2151: STORM-2542: Remove KafkaConsumer.subscribe API option, ma...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2151
@harshach Yes, sorry I thought I'd make a separate JIRA for that so we could list the deprecation in 1.2.0 and the removal of these classes in 2.0.0. The PR is here https://github.com/apache/storm/pull/2224
---
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.
---