Skip to content

Commit offsets when all the records in a poll are skipped by interceptors #2372

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

Closed
lobodpav opened this issue Aug 1, 2022 · 2 comments · Fixed by #2379
Closed

Commit offsets when all the records in a poll are skipped by interceptors #2372

lobodpav opened this issue Aug 1, 2022 · 2 comments · Fixed by #2379

Comments

@lobodpav
Copy link

lobodpav commented Aug 1, 2022

Expected Behavior

Offsets shall be committed even if all records in a poll are skipped by interceptors.

Current Behavior

Offsets are not committed when all records in a poll are skipped by interceptors, causing lags.

Context

In our project, we skip huge amounts of records (company-wide topics where only a fraction is important to us).
Therefore, polls often contain records that are all skipped by interceptors.
In such cases, partition offsets are not committed and we observe enormous lags (hundreds of thousands).

This behaviour effectively prevents us from detecting slow consumers.

The only workaround is to avoid interceptors and implement a custom CompositeRecordFilterStrategy that does the same thing.

However, we need to have our custom LoggingInterceptor in place (begins a new XRay Segment on intercept and ends it on afterRecord) that logs only non-skipped records.

Since the interceptors are called prior to the filters, we're forced into an ugly workaround where the LoggingInterceptor will have to call the CompositeRecordFilterStrategy to verify if a message will be filtered out and log those non-filtered.

This issue is created after a discussion with @garyrussell on StackOverflow.

@garyrussell
Copy link
Contributor

ugly workaround

It might be a little less "ugly" if you use a single class that implements both interfaces.

@lobodpav
Copy link
Author

lobodpav commented Aug 1, 2022

ugly workaround

It might be a little less "ugly" if you use a single class that implements both interfaces.

Good idea, will give it a try!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants