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

Fix: function with multi-topic not acking on effectively-once #2347

Merged
merged 3 commits into from Aug 13, 2018

Conversation

rdhabalia
Copy link
Contributor

Motivation

MultiTopicsConsumerImpl doesn't support acknowledgeCumulativeAsync and therefore, function with multi-topic and EFFECTIVELY_ONCE processing is not acking message and failing EFFECTIVELY_ONCE behavior.

Modifications

Function should ack message for a specific topic consumer if inputTopicConsumer is multi-topic consumer.

Result

Function should able to ack messages for multi-topic consumer when processing-guarantee is EFFECTIVELY_ONCE

@rdhabalia rdhabalia added type/bug The PR fixed a bug or issue reported a bug area/function labels Aug 9, 2018
@rdhabalia rdhabalia added this to the 2.1.1-incubating milestone Aug 9, 2018
@rdhabalia rdhabalia self-assigned this Aug 9, 2018
if (pulsarSourceConfig
.getProcessingGuarantees() == FunctionConfig.ProcessingGuarantees.EFFECTIVELY_ONCE) {
// try to find actual consumer of the messageId
if (inputConsumer instanceof MultiTopicsConsumerImpl) {
Copy link
Member

Choose a reason for hiding this comment

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

does this mean ackCumulativeAsync is not working in MultiTopicsConsumerImpl? If so, should this be fixed in MultiTopicsConsumerImpl instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If so, should this be fixed in MultiTopicsConsumerImpl instead?

Yeah, earlier I was thinking about ordering but MultiConsumer anyway doesn't maintain ordering among the consumers and we can also do cumulative ack for individual consumer. let me fix it.

@rdhabalia
Copy link
Contributor Author

@sijie fixed it.

@rdhabalia
Copy link
Contributor Author

rerun java8 tests

@rdhabalia
Copy link
Contributor Author

@sijie @merlimat can we merge this PR.?

@sijie sijie merged commit 0d2154e into apache:master Aug 13, 2018
@sijie
Copy link
Member

sijie commented Aug 13, 2018

@rdhabalia merged.

I also changed the milestone to 2.2.0 since it is merged to master. when it is cherry-picked to 2.1.1, will update the milestone. otherwise, let's assume the changes based on master will be parted to major releases.

@rdhabalia
Copy link
Contributor Author

sure.. 👍

@rdhabalia rdhabalia deleted the multi_ack branch August 13, 2018 21:21
sijie pushed a commit to sijie/pulsar that referenced this pull request Aug 27, 2018
…#2347)

 ### Motivation

`MultiTopicsConsumerImpl` doesn't support `acknowledgeCumulativeAsync` and therefore, function with multi-topic and `EFFECTIVELY_ONCE` processing is not acking message and failing `EFFECTIVELY_ONCE` behavior.

 ### Modifications

Function should ack message for a specific topic consumer if `inputTopicConsumer` is multi-topic consumer.

 ### Result

Function should able to ack messages for multi-topic consumer when processing-guarantee is `EFFECTIVELY_ONCE`
@sijie
Copy link
Member

sijie commented Aug 27, 2018

merged 068db59 into branch-2.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/function type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants