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

Update protection info on recovered logs data #9875

Closed
wants to merge 17 commits into from

Conversation

akomurav
Copy link
Contributor

@akomurav akomurav commented Apr 20, 2022

Update protection info on recovered logs data

Test Plan:

  • Benchmark setup: TEST_TMPDIR=/dev/shm/100MB_WAL_DB/ ./db_bench -benchmarks=fillrandom -write_buffer_size=1048576000
  • Benchmark command: TEST_TMPDIR=/dev/shm/100MB_WAL_DB/ /usr/bin/time ./db_bench -use_existing_db=true -benchmarks=overwrite -write_buffer_size=1048576000 -writes=1 -report_open_timing=true
  • Results before this PR
OpenDb:     2350.14 milliseconds
OpenDb:     2296.94 milliseconds
OpenDb:     2184.29 milliseconds
OpenDb:     2167.59 milliseconds
OpenDb:     2231.24 milliseconds
OpenDb:     2109.57 milliseconds
OpenDb:     2197.71 milliseconds
OpenDb:     2120.8 milliseconds
OpenDb:     2148.12 milliseconds
OpenDb:     2207.95 milliseconds
  • Results after this PR
OpenDb:     2424.52 milliseconds
OpenDb:     2359.84 milliseconds
OpenDb:     2317.68 milliseconds
OpenDb:     2339.4 milliseconds
OpenDb:     2325.36 milliseconds
OpenDb:     2321.06 milliseconds
OpenDb:     2353.98 milliseconds
OpenDb:     2344.64 milliseconds
OpenDb:     2384.09 milliseconds
OpenDb:     2428.58 milliseconds

Mean regressed 7.2% (2201.4 -> 2359.9)

gonzus and others added 4 commits April 18, 2022 23:57
Summary:
Implement key-value checksums on data recovered from the WAL.

Test Plan:
TBD

Reviewers:
Andrew Kryczka

Subscribers:

Tasks:
T112036080

Tags:
WAL
Summary:
Changes originated in suggestions from ajkr.

Test Plan:
TBD

Reviewers:
Andrew Kryczka

Subscribers:

Tasks:
T112036080

Tags:
WAL
db/write_batch.cc Outdated Show resolved Hide resolved
db/write_batch.cc Outdated Show resolved Hide resolved
@ajkr
Copy link
Contributor

ajkr commented Apr 28, 2022

Added my own benchmark to description. I think the regression (7.2% on in-memory WAL) should be OK to tolerate without a flag for this feature considering it's crash-recovery only. Let me know if anyone disagrees.

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

IMO it'd be good to try the alternative approach suggested for handling prot_info_idx_ updates and see if it's cleaner. Otherwise LGTM.

db/write_batch.cc Show resolved Hide resolved
db/write_batch.cc Outdated Show resolved Hide resolved
db/db_impl/db_impl_open.cc Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@akomurav has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@akomurav has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@akomurav has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@akomurav akomurav changed the title Checksum recovered Update protection info on recovered logs data Apr 28, 2022
@facebook-github-bot
Copy link
Contributor

@akomurav has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@akomurav has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

5 participants