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 error in commitOffsets #369

Merged
merged 1 commit into from
Nov 29, 2018
Merged

Conversation

michele-brambilla
Copy link

@michele-brambilla michele-brambilla commented Nov 28, 2018

Description of work

The KafkaW::Consumer throws an error if rd_kafka_commit returns something different from RD_KAFKA_RESP_ERR_NO_ERROR. This happens for example when there are no older messages (eventually within the specified time window) in the log. According to the discussion in this ticket though

_NO_OFFSET is a "soft error", it means it didnt have to commit the offset because the current offset had already been committed.

With this PR RD_KAFKA_RESP_ERR__NO_OFFSET is not considered an error, but a waning is emitted.

Issue

DM-1197

Other

Can someone please give it a try?


Code Review (To be filled in by the reviewer only)

  • Is the code of an acceptable quality?
  • Do the changes function as described and is it robust?

Nominate for Group Code Review (Anyone can nominate it)

Indicate if you think the code should be reviewed in a Thursday code review session.

  • Recommend for group code review

Also, nominate it on the code_review Slack channel (does someone want to automate this?).

Copy link
Contributor

@matthew-d-jones matthew-d-jones left a comment

Choose a reason for hiding this comment

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

The change looks fine, but do we need to commit offsets at all?
In Mantid I set the following configuration and don't commit offsets:

conf->set("enable.auto.commit", "false", errorMsg);
conf->set("enable.auto.offset.store", "false", errorMsg);
conf->set("offset.store.method", "none", errorMsg);

@mattclarke mattclarke merged commit 58fc288 into master Nov 29, 2018
@mattclarke mattclarke deleted the dm1197_subscribe_to_timestamp branch November 29, 2018 08:12
@dominikwerder
Copy link
Contributor

Is there a test case which demonstrates these possibilities, or how should one set up such a test?

@matthew-d-jones
Copy link
Contributor

matthew-d-jones commented Nov 29, 2018

@dominikwerder Not really, I wouldn't expect anything to go wrong when setting these, therefore I can't suggest what to test for. The only possibility for it being required, that I can think of, is during partition rebalance. But given that we aren't making use of consumer groups, I'm not sure why we have the rebalance callback either...
Michal and Jack are making other changes to the Kafka interface so the most sensible course of action is probably to try removing offset commiting at the same time, because we'll need to do long-running, rigorous testing of those changes anyway.

@dominikwerder
Copy link
Contributor

👍

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