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 DeltaLayer dumping #5045

Merged
merged 5 commits into from
Aug 18, 2023
Merged

Fix DeltaLayer dumping #5045

merged 5 commits into from
Aug 18, 2023

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Aug 18, 2023

Problem

Before, DeltaLayer dumping (via cargo run --release -p pagectl -- print-layer-file ) would crash as one can't call Handle::block_on in an async executor thread.

Summary of changes

Avoid the problem by using DeltaLayerInner::load_keys to load the keys into RAM (which we already do during compaction), and then load the values one by one during dumping.

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
  • If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

  • Do not forget to reformat commit message to not include the above checklist

Before, it would crash as one couldn't call Handle::block_on
within an async executor thread. Now, we just use
DeltaLayerInner::load_keys to load the keys into RAM, and
then load the values one by one during dumping.
@arpad-m arpad-m requested review from a team as code owners August 18, 2023 21:01
@arpad-m arpad-m requested review from lubennikovaav and skyzh and removed request for a team August 18, 2023 21:01
@arpad-m
Copy link
Member Author

arpad-m commented Aug 18, 2023

FTR, I confirmed locally that the test caused a crash before this patch.

@github-actions
Copy link

github-actions bot commented Aug 18, 2023

1624 tests run: 1549 passed, 0 failed, 75 skipped (full report)


The comment gets automatically updated with the latest test results
5f99267 at 2023-08-18T22:23:48.116Z :recycle:

@arpad-m arpad-m merged commit a23b077 into main Aug 18, 2023
@arpad-m arpad-m deleted the arpad/fix_delta_dumping branch August 18, 2023 22:56
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.

2 participants