Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STORM-822 Implement Kafka 0.9 consumer API #986

Closed
wants to merge 6 commits into from

Conversation

sergeyevd
Copy link

Hello for my own needs I've created new Spout that uses new Kafka Consumer (0.9) Java API. It could help you as part of STORM-822.

It works with existing storm-kafka module and KafkaBolt.

@connieyang
Copy link

That's great! When will this pull request be merged to master? Also, will the Trident API work with the latest chagnes?

@revans2
Copy link
Contributor

revans2 commented Jan 6, 2016

CI seems to be failing with a trident kafka test hanging.

The code appears to provide a best effort replay, but that is neither at least once nor at most once, so it is likely to cause issues for some topologies that don't expect this a-typical behavior. I am not an expert on the kafka API, but it looks like auto.commit.enable is inherently incompatible with what storm wants from a well behaved spout.

@harshach
Copy link
Contributor

harshach commented Jan 6, 2016

@revans2 @connieyang although this is a good start I don't see it handles lot of cases that current storm-kafka handles. @hmcl is working on a full-fledged integration of new kafka consumer api. You should see his PR soon.

@connieyang
Copy link

Excellent! I will follow @hmcl for his PR. Thanks for the update!

@sergeyevd
Copy link
Author

Thanks for comments! @harshach could you please give me ETA of @hmcl PR with this feature? I want to be sure, that new storm-kafka will work nicely with my existing code base. Also, I can write non-autocommit approach by next two days, if its needed.
Have a nice day!

@hmcl
Copy link
Contributor

hmcl commented Jan 8, 2016

@Deepnekroz I am working on this. We wil make sure that all works well with what you have.

@sergeyevd
Copy link
Author

Hello again! Here's non-autocommit implementation of Kafka 0.9 spout.

@darionyaphet
Copy link
Contributor

hi @Deepnekroz using difference version kafka maybe use difference spout ?

kafka 0.8.X is KafkaSpout and kafka 0.9 is KafkaJavaApiSpout ?

@sergeyevd
Copy link
Author

Hello @darionyaphet ! Yes, KafkaJavaApiSpout is using Kafka 0.9 Consumer API, and KafkaSpout is retained for backward compatibility. It also will work with Kafka 0.9, because they left old API, but it is deprecated for now.

@darionyaphet
Copy link
Contributor

I think the two kind of spout could be merge into one ?

@tgravescs
Copy link

@hmcl just curious your status on this, are you actively working on it and do you have eta? thanks!

@hmcl
Copy link
Contributor

hmcl commented Jan 19, 2016

@tgravescs I am actively working on this and I am going to conclude my implementation regardless of the status of the proposed patches. This task was and still is my priority, however, due to an emergency of a team member, I had to interrupt working on this to cover for him. I will resume as soon as possible.

@erikdw
Copy link
Contributor

erikdw commented Jan 20, 2016

@tgravescs, @hmcl, and @Deepnekroz : @hsun-cnnxty is also working on a refactor of the kafka-spout, to support the new ZooKeeper-free kafka API. Can y'all please sync up to ensure we have a consistent story after these efforts?

@ooasis
Copy link

ooasis commented Jan 20, 2016

I don't have much experience on how the collaborations be done in storm project. Happy to take any advices on what I can help.

@revans2
Copy link
Contributor

revans2 commented Jan 20, 2016

@hsun-cnnxty
I am not sure that we have had much in the way of collaboration in the past, so if others have suggestions on this that would be great.

I personally think that first we need to define the goals of each person involved here so we can be sure to know what features are needed to declare this done. We can divide different features up into smaller pieces/JIRA that can be done independently if that is simpler.

STORM-822 talks about using the kafka 0.8.3 API, not the 0.9.0 API, and that is it (I assume that they are the same and a version number was changed before release). Hence the need to better define what STORM-822 is intended to cover.

So from my perspective the minimum goals that I have for a 0.9.0 API Spout is

  • It needs to support at least once processing correctly
  • It needs to support at most once processing correctly (or at least as well as the previous spout did)
  • it needs to support the security APIs

Before we can remove the old spout though it also needs to support the same functionality. Not necessarily a drop in replacement (Although that would be nice too).

@hmcl @tgravescs @erikdw @Deepnekroz @harshach what are the requirements that you have? Once we know the requirements we can add them to STORM-822, setup a plan a file subtasks for the different parts of the plan, ideally with dependencies.

I personally don't want to run the show in coordinating this. I am up to my eyeballs in 1.x/JStorm, so if someone else here wants to volunteer to help coordinate things that would be great.

@ooasis
Copy link

ooasis commented Jan 20, 2016

@revans2 and all,

I feel my PR ([STORM-1015]: Allow Kafka offsets to be saved using Kafka's consumer offset management api) is kind of independent of this PR as it is not related to the Kafka consumer API changes. I don't see major changes in consumer offset API in 0.9.0 so my feeling is that once you guys have the 0.9.0 branch ready, I can just merge in my change.

I am curious to know what is the plan to support both 0.8.x and 0.9.x in coming releases, or what is the general strategy to support incompatible Kafka versions in the future.

Another question that is not related to this topic, but maybe I can get help from you. How does the cross compiling of Scala version 2.10, 2.11 works in the build process? I cannot figure out how to do it locally on my laptop.

-thanks

@connieyang
Copy link

@hmcl Will your PR include the rev for Trident API to work with Kafka 0.9?

@hmcl
Copy link
Contributor

hmcl commented Jan 21, 2016

@connieyang the goal of my patch is to entirely replace the existing kafka spout; that includes the Trident API. In order to allow for a smooth migration, as well as avoid any backward compatibility issues, I started writing the code in a new package, completely isolated from the existing one. The goal is to do a complete rewrite. It is my opinion that this task should be properly designed to address future requirements, including proper testing and backwards compatibility. However, some patches have recently come in addressing the JIRAs I had assigned (despite my comments), which will inevitably lead to wasted effort by either one of the parts.

Nevertheless, I will do my best to make the most of this situation for the benefit of the community.

@revans2
Copy link
Contributor

revans2 commented Jan 21, 2016

@hsun-cnnxty I agree that STORM-1015 looks independent of the other changes happening to support the 0.9 API. It adds in a new feature for backwards compatibility though.

@connieyang
Copy link

I too agree that STORM-1015 is beyond the scope of Kafka 0.9 uprev related changes. So, I think it makes sense to keep them separate.

@hmcl, any ETA on the rewrite or uprev effort? Thanks much!

@hmcl
Copy link
Contributor

hmcl commented Jan 25, 2016

@connieyang I will try to have a patch for review by the end of this week. I will also try to keep you posted on the progress. Thanks!

@jianbzhou
Copy link

Hi @hmcl, this is Wayne from connieyang's team, not sure could you please provide a patch for us to kick off the testing? We are doing the QA testing this week, it would be great if I can add your patch in asap...many thanks!!!

@connieyang
Copy link

@hmcl, any update to your patch? Thanks!

@hmcl
Copy link
Contributor

hmcl commented Feb 9, 2016

Hi @connieyang @jianbzhou apologies for not replying to you earlier. I was sidetracked with a last minute release requirement that wasn't planned that got things a bit off schedule. I am available to resume this task again this week. I will upload a patch by the end of the week.

@jianbzhou
Copy link

Hi @hmcl ,may I have the patch please? Thanks!

@hmcl
Copy link
Contributor

hmcl commented Feb 16, 2016

@jianbzhou I am just finishing the final touches. I will uploaded it today.

@jianbzhou
Copy link

Hi *@hmcl *, could you please share the patch please? thanks!

@hmcl
Copy link
Contributor

hmcl commented Feb 18, 2016

@jianbzhou sorry it took me a bit longer. Trying my best to put it up within the next few hours.

@jianbzhou
Copy link

*@hmcl * thanks a lot i understand the complexity. Appreciate your help and it would be great to see your patch by EOD today your time.

@tgravescs
Copy link

@hmcl Sorry to keep hounding on this but how close are you? If you are just doing some final cleanup or have a few minor issues could you post what you have so we can start testing and taking a look. We have customers waiting on this.

@jianbzhou
Copy link

Hi *@hmcl *, is there any update on the patch please? thanks.

@hmcl
Copy link
Contributor

hmcl commented Feb 21, 2016

@connieyang @jianbzhou @tgravescs patch is here:
#1131

d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
We are closing stale Pull Requests to make the list more manageable.

Please re-open any Pull Request that has been closed in error.

Closes apache#608
Closes apache#639
Closes apache#640
Closes apache#648
Closes apache#662
Closes apache#668
Closes apache#692
Closes apache#705
Closes apache#724
Closes apache#728
Closes apache#730
Closes apache#753
Closes apache#803
Closes apache#854
Closes apache#922
Closes apache#986
Closes apache#992
Closes apache#1019
Closes apache#1040
Closes apache#1041
Closes apache#1043
Closes apache#1046
Closes apache#1051
Closes apache#1078
Closes apache#1146
Closes apache#1164
Closes apache#1165
Closes apache#1178
Closes apache#1213
Closes apache#1225
Closes apache#1258
Closes apache#1259
Closes apache#1268
Closes apache#1272
Closes apache#1277
Closes apache#1278
Closes apache#1288
Closes apache#1296
Closes apache#1328
Closes apache#1342
Closes apache#1353
Closes apache#1370
Closes apache#1376
Closes apache#1391
Closes apache#1395
Closes apache#1399
Closes apache#1406
Closes apache#1410
Closes apache#1422
Closes apache#1427
Closes apache#1443
Closes apache#1462
Closes apache#1468
Closes apache#1483
Closes apache#1506
Closes apache#1509
Closes apache#1515
Closes apache#1520
Closes apache#1521
Closes apache#1525
Closes apache#1527
Closes apache#1544
Closes apache#1550
Closes apache#1566
Closes apache#1569
Closes apache#1570
Closes apache#1575
Closes apache#1580
Closes apache#1584
Closes apache#1591
Closes apache#1600
Closes apache#1611
Closes apache#1613
Closes apache#1639
Closes apache#1703
Closes apache#1711
Closes apache#1719
Closes apache#1737
Closes apache#1760
Closes apache#1767
Closes apache#1768
Closes apache#1785
Closes apache#1799
Closes apache#1822
Closes apache#1824
Closes apache#1844
Closes apache#1874
Closes apache#1918
Closes apache#1928
Closes apache#1937
Closes apache#1942
Closes apache#1951
Closes apache#1957
Closes apache#1963
Closes apache#1964
Closes apache#1965
Closes apache#1967
Closes apache#1968
Closes apache#1971
Closes apache#1985
Closes apache#1986
Closes apache#1998
Closes apache#2031
Closes apache#2032
Closes apache#2071
Closes apache#2076
Closes apache#2108
Closes apache#2119
Closes apache#2128
Closes apache#2142
Closes apache#2174
Closes apache#2206
Closes apache#2297
Closes apache#2322
Closes apache#2332
Closes apache#2341
Closes apache#2377
Closes apache#2414
Closes apache#2469
d2r pushed a commit to d2r/storm that referenced this pull request Oct 16, 2018
We are closing stale Pull Requests to make the list more manageable.

Please re-open any Pull Request that has been closed in error.

Closes apache#608
Closes apache#639
Closes apache#640
Closes apache#648
Closes apache#662
Closes apache#668
Closes apache#692
Closes apache#705
Closes apache#724
Closes apache#728
Closes apache#730
Closes apache#753
Closes apache#803
Closes apache#854
Closes apache#922
Closes apache#986
Closes apache#992
Closes apache#1019
Closes apache#1040
Closes apache#1041
Closes apache#1043
Closes apache#1046
Closes apache#1051
Closes apache#1078
Closes apache#1146
Closes apache#1164
Closes apache#1165
Closes apache#1178
Closes apache#1213
Closes apache#1225
Closes apache#1258
Closes apache#1259
Closes apache#1268
Closes apache#1272
Closes apache#1277
Closes apache#1278
Closes apache#1288
Closes apache#1296
Closes apache#1328
Closes apache#1342
Closes apache#1353
Closes apache#1370
Closes apache#1376
Closes apache#1391
Closes apache#1395
Closes apache#1399
Closes apache#1406
Closes apache#1410
Closes apache#1422
Closes apache#1427
Closes apache#1443
Closes apache#1462
Closes apache#1468
Closes apache#1483
Closes apache#1506
Closes apache#1509
Closes apache#1515
Closes apache#1520
Closes apache#1521
Closes apache#1525
Closes apache#1527
Closes apache#1544
Closes apache#1550
Closes apache#1566
Closes apache#1569
Closes apache#1570
Closes apache#1575
Closes apache#1580
Closes apache#1584
Closes apache#1591
Closes apache#1600
Closes apache#1611
Closes apache#1613
Closes apache#1639
Closes apache#1703
Closes apache#1711
Closes apache#1719
Closes apache#1737
Closes apache#1760
Closes apache#1767
Closes apache#1768
Closes apache#1785
Closes apache#1799
Closes apache#1822
Closes apache#1824
Closes apache#1844
Closes apache#1874
Closes apache#1918
Closes apache#1928
Closes apache#1937
Closes apache#1942
Closes apache#1951
Closes apache#1957
Closes apache#1963
Closes apache#1964
Closes apache#1965
Closes apache#1967
Closes apache#1968
Closes apache#1971
Closes apache#1985
Closes apache#1986
Closes apache#1998
Closes apache#2031
Closes apache#2032
Closes apache#2071
Closes apache#2076
Closes apache#2108
Closes apache#2119
Closes apache#2128
Closes apache#2142
Closes apache#2174
Closes apache#2206
Closes apache#2297
Closes apache#2322
Closes apache#2332
Closes apache#2341
Closes apache#2377
Closes apache#2414
Closes apache#2469
@asfgit asfgit closed this in #2880 Oct 22, 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.

10 participants