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

Consumer group support for manually comitting offsets #1699

Merged
merged 1 commit into from
Jun 24, 2020

Conversation

wclaeys
Copy link
Contributor

@wclaeys wclaeys commented May 11, 2020

Provide manual Commit method and don't create mainloop if AutoCommit is disabled

Fixes #1570

@knoguchi
Copy link

I'm just a user but how are you going to return errors?

@wclaeys
Copy link
Contributor Author

wclaeys commented May 11, 2020

In the same manner as how it's now being done in the 'mainLoop' when AutoCommit.Enable is true.

@BPScott
Copy link

BPScott commented May 15, 2020

Hi @wclaeys, your CLA check is failing because your commits aren't associated with your Github account. You need to connect your email address with your github account per https://help.github.com/en/github/setting-up-and-managing-your-github-user-account/adding-an-email-address-to-your-github-account.

@wclaeys
Copy link
Contributor Author

wclaeys commented May 15, 2020

@BPScott , thank you for your message. I've updated my profile.

@ghost ghost removed the cla-needed label May 15, 2020
Copy link
Contributor

@d1egoaz d1egoaz left a comment

Choose a reason for hiding this comment

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

Thanks! 🚀

Could you please add some test for this!

@wclaeys wclaeys requested a review from d1egoaz May 23, 2020 17:58
@Gypsying
Copy link

Gypsying commented Jun 2, 2020

Is this OK ?
What time will merge the pr?

@Morriaty-The-Murderer
Copy link

Hi guys, how is this pr going now?

@coldfire-x
Copy link

fixed yet?

Copy link
Contributor

@d1egoaz d1egoaz left a comment

Choose a reason for hiding this comment

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

just some small comments.
Thanks

offset_manager_test.go Outdated Show resolved Hide resolved
offset_manager_test.go Outdated Show resolved Hide resolved
@d1egoaz
Copy link
Contributor

d1egoaz commented Jun 23, 2020

this looks LGTM, but I'd love to have another 👍 from another Sarama maintainer @bai @dnwe @varun06 @mimaison @edoardocomar

Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

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

Thanks for working on this PR @wclaeys — the changes overall look good to me, a couple of thoughts below:

consumer_group.go Show resolved Hide resolved
consumer_group.go Show resolved Hide resolved
@dnwe dnwe changed the title Provide manual Commit method and don't create mainloop if AutoCommit is disabled Consumer group support for manually comitting offsets Jun 23, 2020
@ghost ghost added the cla-needed label Jun 24, 2020
@wclaeys
Copy link
Contributor Author

wclaeys commented Jun 24, 2020

Can somebody please cancel this new CLA check? Accidentally pushed without a proper e-mail setting (reverted it and did a proper commit). @BPScott maybe? Thank you!

@dnwe
Copy link
Collaborator

dnwe commented Jun 24, 2020

@wclaeys I think the PR is ready, so why don’t you squash your commits now and force push again with your correct email, that should pass the CLA check

@dnwe
Copy link
Collaborator

dnwe commented Jun 24, 2020

@wclaeys you need to rebase your branch on master to get the checks to pass

This branch is 1 commit ahead, 24 commits behind Shopify:master.

- expose a `Commit()` sync method on ConsumerGroupSession
- don't create mainLoop in OffsetManager unless AutoCommit is enabled
@dnwe dnwe force-pushed the master branch 2 times, most recently from 0cba20c to cb9d1e8 Compare June 24, 2020 08:07
@dnwe
Copy link
Collaborator

dnwe commented Jun 24, 2020

@wclaeys ah don't worry, I found that I had permission to push to your branch myself so I performed the rebase. If the checks both pass green then I will go ahead and merge. Thanks for your contribution on this one, really appreciated 👍

@dnwe
Copy link
Collaborator

dnwe commented Jun 24, 2020

As there's been a lot of interest in this functionality, it would be great if all the people watching this PR and the accompanying issues could test it out with their application code once it's merged, in advance of us cutting any new release version.

Assuming you're using go modules to manage your dependencies, once this has merged, you can update your go.mod to fetch the latest commit from the default branch by doing:

go get github.com/Shopify/sarama@HEAD

@bglmmz
Copy link

bglmmz commented Jul 20, 2020

How about a single consumer? Could a single consumer commit offsets manually?
Maybe we can create a consumer group it only has a single consumer instance.

@aneeskA
Copy link

aneeskA commented Aug 4, 2020

As there's been a lot of interest in this functionality, it would be great if all the people watching this PR and the accompanying issues could test it out with their application code once it's merged, in advance of us cutting any new release version.

Assuming you're using go modules to manage your dependencies, once this has merged, you can update your go.mod to fetch the latest commit from the default branch by doing:

go get github.com/Shopify/sarama@HEAD

@dnwe Is there any timeline for releasing this feature?

@alok87
Copy link

alok87 commented Aug 7, 2020

Please suggest when are you releasing this, this helpful in cases where you want the commit to happen instantly, systems which care about order and duplicates really need it.

alok87 added a commit to practo/tipoca-stream that referenced this pull request Aug 9, 2020
@jameshalsall
Copy link

doesn't this break backwards compatibility with the addition of the new interface method?

@d1egoaz
Copy link
Contributor

d1egoaz commented Aug 19, 2020

@jameshalsall yup

I think we could apply this trick if we don't want users to implement the interface

    // A private method to prevent users implementing the
    // interface and so future additions to it will not
    // violate Go 1 compatibility.
    private()

from https://blog.golang.org/module-compatibility

what do you think @dnwe ? ]

this PR was also merged adding a new method to an existing interface #1781

@jameshalsall
Copy link

jameshalsall commented Aug 19, 2020 via email

@hipper
Copy link

hipper commented Nov 26, 2020

Hey @wclaeys , found a bug that looks like introduced after this change. The problem is when you close the consumer it ignores the AutoCommit flag and commits the offset before it exits. That problem doesn't exists in 1.26.4

Described more here: #1843

alok87 added a commit to practo/tipoca-stream that referenced this pull request Dec 15, 2020
alok87 added a commit to practo/tipoca-stream that referenced this pull request Dec 15, 2020
alok87 added a commit to practo/tipoca-stream that referenced this pull request Jun 5, 2021
alok87 added a commit to practo/tipoca-stream that referenced this pull request Jun 7, 2021
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.

commit offset manually when using consumer group