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

refactor(walredo): use heavier_once_cell to store the process handle #6595

Conversation

problame
Copy link
Contributor

@problame problame commented Feb 2, 2024

In preceding PR #6589 we made heavier_once_cell use an RwLock, so, now we can switch walredo to use it without introducing a performance regression.

I want to switch to heavier_once_cell because we'll replace the WalRedoProcess::launch() call with an async fn call when we switch to a pre-spawned pool of walredo processes in PR #6596 .

part of #6581

When we'll later introduce a global pool of pre-spawned walredo
processes (#6581), this
refactoring avoids plumbing through the reference to the pool to all the
places where we create a broken tenant.

Builds atop the refactoring in #6583
…e_cell

The API is nice, exactly what we want, but we would want a more
optimistic underlying sync primitive.
… problame/2024-02-walredo-work/prespawn/broken-tenants-no-walredo
…o-walredo' into problame/2024-02-walredo-work/prespawn/heaver-once-cell-for-process-launch
Using the RwLock reduces contention on the hot path.
… problame/2024-02-walredo-work/prespawn/broken-tenants-no-walredo
…o-walredo' into problame/2024-02-walredo-work/prespawn/heaver-once-cell-for-process-launch
… problame/2024-02-walredo-work/prespawn/broken-tenants-no-walredo
…o-walredo' into problame/2024-02-walredo-work/prespawn/heaver-once-cell-for-process-launch
…-for-process-launch' into problame/2024-02-walredo-work/prespawn/switch-to-heavier-once-cell-with-rwlock
@problame problame marked this pull request as ready for review February 2, 2024 18:37
@problame problame requested a review from a team as a code owner February 2, 2024 18:37
@problame problame changed the title refactor(walredo): use heavier_once_cell to store the process refactor(walredo): use heavier_once_cell to store the process handle Feb 2, 2024
Copy link

github-actions bot commented Feb 2, 2024

2388 tests run: 2274 passed, 0 failed, 114 skipped (full report)


Flaky tests (2)

Postgres 15

  • test_crafted_wal_end[last_wal_record_crossing_segment]: release

Postgres 14

  • test_deletion_queue_recovery[no-validate-keep]: debug

Code coverage (full report)

  • functions: 54.3% (11305 of 20830 functions)
  • lines: 81.2% (63441 of 78083 lines)

The comment gets automatically updated with the latest test results
3ec02d1 at 2024-02-06T17:35:17.693Z :recycle:

Base automatically changed from problame/2024-02-walredo-work/prespawn/heaver-once-cell-for-process-launch to main February 6, 2024 12:04
@problame
Copy link
Contributor Author

problame commented Feb 6, 2024

This should be ready for merge

@arpad-m
Copy link
Member

arpad-m commented Feb 12, 2024

Given #6704 do you still pursue this @problame ?

@problame
Copy link
Contributor Author

Yes, I still need something like this.

If we can't make tokio::sync::RwLock-based heavier_once_cell work, I'll use a tokio::sync::RwLock myself in this PR.

@problame
Copy link
Contributor Author

Note this PR is part of #6581 which is currently on hold due to other priorities.

@problame
Copy link
Contributor Author

problame commented Apr 4, 2024

this is stale and not directly applicable because we had to revert the heavier_once_cell changes: #6704

@problame problame closed this Apr 4, 2024
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