Skip to content

Conversation

@srdo
Copy link
Contributor

@srdo srdo commented Jun 5, 2017

This builds on #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.

@srdo
Copy link
Contributor Author

srdo commented Jun 5, 2017

If this goes in I'll follow up with a PR against 1.x to deprecate the classes using KafkaConsumer.subscribe

@hmcl
Copy link
Contributor

hmcl commented Jun 7, 2017

@srdo was this agreed upon ?

@srdo
Copy link
Contributor Author

srdo commented Jun 7, 2017

@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.

@hmcl
Copy link
Contributor

hmcl commented Jun 29, 2017

@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, the 2nd commit STORM-2541 PR, plus its own commit moving classes into the appropriate packages.

@hmcl
Copy link
Contributor

hmcl commented Jun 29, 2017

@srdo Thanks for your diligence and awesome work refactoring this code. It just made it much better.

@srdo srdo force-pushed the STORM-2542 branch 2 times, most recently from 548e938 to f473b5d Compare July 4, 2017 22:05
Copy link
Contributor

@hmcl hmcl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.
@srdo I made only one minor comment in the documentation.
@harshach 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.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@harshach
Copy link
Contributor

+1. Thanks @srdo this looks great.

@srdo srdo force-pushed the STORM-2542 branch 7 times, most recently from 269e24d to 3f44dca Compare July 18, 2017 21:53
@asfgit asfgit merged commit fdb649e into apache:master Jul 19, 2017
@srdo
Copy link
Contributor Author

srdo commented Jul 19, 2017

Thanks for the reviews.

@harshach
Copy link
Contributor

@srdo are we not planning on pushing this into 1.x-branch?

@srdo
Copy link
Contributor Author

srdo commented Jul 21, 2017

@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 #2224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants