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 table restoration for compacted changelogs #373

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

wbarnha
Copy link
Member

@wbarnha wbarnha commented Sep 29, 2022

Fixes #176.

patkivikram
patkivikram previously approved these changes Sep 29, 2022
@wbarnha wbarnha changed the title Remove offset - 1 from _build_offsets Remove offset - 1 from _build_offsets to fix table restoration Sep 29, 2022
@wbarnha wbarnha requested a review from patkivikram September 29, 2022 14:25
faust/tables/recovery.py Outdated Show resolved Hide resolved
@wbarnha wbarnha changed the title Remove offset - 1 from _build_offsets to fix table restoration Fix table restoration for compacted changelogs Sep 29, 2022
@wbarnha
Copy link
Member Author

wbarnha commented Nov 22, 2022

@elrik75 how about giving this a try? Apparently the offset - 1 logic was around since the beginning of the project and I changed a unit test to accommodate this.

@codecov-commenter
Copy link

codecov-commenter commented Nov 22, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.09%. Comparing base (ff75c0b) to head (e9aaebb).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #373   +/-   ##
=======================================
  Coverage   94.09%   94.09%           
=======================================
  Files         102      102           
  Lines       11100    11100           
  Branches     1196     1196           
=======================================
  Hits        10444    10444           
  Misses        557      557           
  Partials       99       99           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wbarnha wbarnha marked this pull request as ready for review November 30, 2022 02:57
@wbarnha
Copy link
Member Author

wbarnha commented Nov 30, 2022

We'll need to test this further before merging.

@wbarnha
Copy link
Member Author

wbarnha commented Nov 30, 2022

I reviewed the code and the problem is much deeper than the changes here.

@wbarnha wbarnha marked this pull request as draft November 30, 2022 22:47
@wbarnha wbarnha closed this Jan 13, 2023
@elrik75
Copy link

elrik75 commented Apr 2, 2023

This bug still exists. I recently lost my rocks-db disk and Faust started and launched the recovery. Once again some changelogs partitions did not start (they have an offset equal to the first Kafka offset - 1). Removing the -1 saved me again. I didn't see any issue after the "fix".

@wbarnha
Copy link
Member Author

wbarnha commented Apr 2, 2023

In that case, I'll reopen the PR as a reminder for me to investigate this.

@wbarnha wbarnha reopened this Apr 2, 2023
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.

Table restore error when the server has compacted the changelog-topic by segment time
5 participants