Skip to content

Add type header to WAL/Checkpoint records and use TSDB records #2436

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

Merged
merged 9 commits into from
Apr 24, 2020

Conversation

codesome
Copy link
Contributor

@codesome codesome commented Apr 9, 2020

What this PR does:

Add a single-byte type header to WAL and Checkpoint records and way to handle old records concurrently. Additionally, replace existing proto records in WAL with Prometheus TSDB records.

Which issue(s) this PR fixes:
Fixes #2426

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
@codesome codesome force-pushed the wal-record-type branch 3 times, most recently from 3ee493f to d5c2e21 Compare April 9, 2020 14:01
@codesome codesome marked this pull request as ready for review April 9, 2020 14:15
@codesome
Copy link
Contributor Author

codesome commented Apr 9, 2020

PR is ready for review

Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

I think we don't handle the migration well. I can't see where we are handling the errors returned by failed proto marshalling

Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

Actually nvm, I missed the merr = nil bits :) It's all good! LGTM!

@codesome
Copy link
Contributor Author

After some offline discussion with Goutham, we decided to add the Prometheus TSDB records in this PR itself to not have many versions or records, as the current record would be obsolete immediately after TSDB records are in.

@codesome codesome changed the title Add type header to WAL and Checkpoint records Apr 14, 2020
@codesome codesome force-pushed the wal-record-type branch 2 times, most recently from f7e26c3 to 5cbf9a4 Compare April 14, 2020 15:03
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome codesome force-pushed the wal-record-type branch 3 times, most recently from 4cc3c02 to 508fc08 Compare April 14, 2020 16:17
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@codesome
Copy link
Contributor Author

I wrote a benchmark for WAL replay (will open PR after #2465 is merged as it depends on that) to compare the new records, and the results look good. Largely depends on Checkpoint/WAL ratio. The benchmark had equal number of samples in WAL and Checkpoint.

benchmark                    old ns/op      new ns/op      delta
BenchmarkWALReplay/#00-8     2027607295     1656442045     -18.31%

benchmark                    old allocs     new allocs     delta
BenchmarkWALReplay/#00-8     3050719        1142134        -62.56%

benchmark                    old bytes      new bytes      delta
BenchmarkWALReplay/#00-8     5943738312     5638734152     -5.13%
Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM with some minor comments. processWAL method seems pretty convoluted, but is not part of this PR.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@pstibrany
Copy link
Contributor

Tests are failing here now.

Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Ganesh Vernekar added 2 commits April 23, 2020 15:33
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
Signed-off-by: Ganesh Vernekar <cs15btech11018@iith.ac.in>
@pstibrany pstibrany merged commit 397bfca into cortexproject:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 participants