Skip to content

Conversation

@srdo
Copy link
Contributor

@srdo srdo commented Mar 10, 2018

https://issues.apache.org/jira/browse/STORM-2974

The changes outside the Trident package are because I'd like us to start using https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/consumer/MockConsumer.java for testing instead of our own mockito mocks. In order to do that we have to refer to the Consumer interface instead of KafkaConsumer. The replacement is a breaking change in a few places. I'm not expecting this to go on 1.x, and backward compatibility was already broken in #2300, so I haven't done any work to provide a smooth transition.

I've tried to follow the transactional spout in storm-kafka to implement this as much as possible.

There are a couple of followup tasks for this that I'll try to get done later, namely https://issues.apache.org/jira/browse/STORM-2990 and https://issues.apache.org/jira/browse/STORM-2991.

@srdo
Copy link
Contributor Author

srdo commented Mar 10, 2018

I have tested the non-opaque spout by running the storm-kafka-client-examples topology, so testing hasn't been very extensive.

@HeartSaVioR
Copy link
Contributor

@srdo
I'm sorry but could you please rebase this? Thanks in advance!

@srdo
Copy link
Contributor Author

srdo commented Jul 10, 2018

@HeartSaVioR Rebased.

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

I should say, thanks for your patience. More than 3 months passed by and we haven't reviewed in time.
I would be +1.

@HeartSaVioR
Copy link
Contributor

I have found that couple of public classes are renamed, but they're in internal package which makes others feeling non-public, so I think we are OK.

@asfgit asfgit merged commit ba52607 into apache:master Jul 13, 2018
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.

3 participants