Skip to content

Fix table restoration for compacted changelogs#373

Draft
wbarnha wants to merge 12 commits into
masterfrom
remove-offset-1
Draft

Fix table restoration for compacted changelogs#373
wbarnha wants to merge 12 commits into
masterfrom
remove-offset-1

Conversation

@wbarnha

@wbarnha wbarnha commented Sep 29, 2022

Copy link
Copy Markdown
Member

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
Comment thread faust/tables/recovery.py Outdated
@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

wbarnha commented Nov 22, 2022

Copy link
Copy Markdown
Member Author

@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

codecov-commenter commented Nov 22, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.09%. Comparing base (ff75c0b) to head (e9aaebb).
⚠️ Report is 1 commits behind head on master.

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 Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

wbarnha commented Nov 30, 2022

Copy link
Copy Markdown
Member Author

We'll need to test this further before merging.

@wbarnha

wbarnha commented Nov 30, 2022

Copy link
Copy Markdown
Member Author

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

elrik75 commented Apr 2, 2023

Copy link
Copy Markdown

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

wbarnha commented Apr 2, 2023

Copy link
Copy Markdown
Member Author

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