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

Ignore NO_OFFSET condition while commiting #108

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

soumyajit-sahu
Copy link

While committing, we should ignore the NO_OFFSET error code.
There is a note in the comments in the rdkafka.h file as follows, which corroborates this change:
If no partitions had valid offsets to commit this callback will be called
with \p err == RD_KAFKA_RESP_ERR__NO_OFFSET which is not to be considered
an error.

@edenhill
Copy link

edenhill commented Jan 4, 2017

The reason for this behaviour is state synchronization: it allows the application to know when an async commit() operation has finished, regardless if it actually had to commit offsets or the requested offsets were only commited (NO_OFFSET).

@soumyajit-sahu
Copy link
Author

We are calling the Consumer.Commit() synchronously to guarantee at-least once in our logic.
I believe that a NO_OFFSET exception is raised when we don't have any new data in any partition. The Consumer.Commit() call should pass in this case too.

@edenhill if we shouldn't ignore the error in the SafeKafkaHandle.Commit(), then would you suggest to:

  1. ignore it in the Consumer.Commit() or,
  2. have the caller ignore it. In this case we can put a comment for Consumer.Commit() to document it just like it is done in rdkafka.h.

I would prefer the later to keep it alike to the c lib.

@edenhill
Copy link

edenhill commented Jan 4, 2017

Yep, I would go with option 2.

@soumyajit-sahu
Copy link
Author

Thanks @edenhill . I have added a comment, and reverted the previous change.

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.

2 participants