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

Cleanup compact_level0_phase1 fsyncing #5852

Merged
merged 3 commits into from
Nov 21, 2023
Merged

Conversation

koivunej
Copy link
Member

While reviewing code noticed a scary layer_paths.pop().unwrap() then realized this should be further asyncified, something I forgot to do when I switched the compact_level0_phase1 back to async in #4938.

This keeps the double-fsync for new deltas as #4749 is still unsolved.

keep the wasteful double-fsync while we have unsolved issues likely
moving the fsyncs around.
@koivunej koivunej requested a review from a team as a code owner November 10, 2023 18:51
@koivunej koivunej requested review from hlinnaka and arpad-m and removed request for a team November 10, 2023 18:51
@koivunej
Copy link
Member Author

Does not conflict with #5234.

Copy link

2376 tests run: 2258 passed, 0 failed, 118 skipped (full report)


Code coverage (full report)

  • functions: 54.6% (8919 of 16330 functions)
  • lines: 81.4% (51297 of 63001 lines)

The comment gets automatically updated with the latest test results
5014267 at 2023-11-10T19:40:03.750Z :recycle:

Copy link
Contributor

@hlinnaka hlinnaka left a comment

Choose a reason for hiding this comment

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

Looks good as far as it goes.

I gather that create_delta_layer still needs the spawn_blocking, until we do #5479. But could we already move the par_fsync() calls out of the spawn_blocking block, and change them into par_fsync_async()? Then we could remove the synchronous par_fsync() function altogether.

@arpad-m
Copy link
Member

arpad-m commented Nov 13, 2023

until we do #5479

I think that more would be needed than re-applying #5291: we'd also need #5824, so actual async for the I/O.

@koivunej koivunej merged commit 0d10992 into main Nov 21, 2023
41 checks passed
@koivunej koivunej deleted the cleanup_compaction_fsyncing branch November 21, 2023 13:30
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.

4 participants