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

Use OnceLock instead of manually implementing it #4805

Merged
merged 1 commit into from
Jul 26, 2023
Merged

Conversation

arpad-m
Copy link
Member

@arpad-m arpad-m commented Jul 25, 2023

Problem

In #4743 , I'm trying to make more of the pageserver async, but in order for that to happen, I need to be able to persist the result of ImageLayer::load across await points. For that to happen, the return value needs to be Send.

Summary of changes

Use OnceLock in the image layer instead of manually implementing it with booleans, locks and Option.

Part of #4743

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

@arpad-m arpad-m requested review from a team as code owners July 25, 2023 13:29
@arpad-m arpad-m requested review from awestover and LizardWizzard and removed request for a team July 25, 2023 13:29
Copy link
Contributor

@awestover awestover left a comment

Choose a reason for hiding this comment

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

looks like it simplified the code a bit! nice.

@github-actions
Copy link

github-actions bot commented Jul 25, 2023

1240 tests run: 1186 passed, 0 failed, 54 skipped (full report)


Flaky tests (7)

Postgres 15

  • test_neon_cli_basics: release
  • test_remote_storage_upload_queue_retries[local_fs]: release
  • test_remote_timeline_client_calls_started_metric[local_fs]: release
  • test_get_tenant_size_with_multiple_branches: release
  • test_empty_tenant_size: release
  • test_s3_wal_replay[local_fs]: debug

Postgres 14

@arpad-m arpad-m force-pushed the arpad/once_cell branch 2 times, most recently from 45cbe4c to 9ffd552 Compare July 26, 2023 00:51
@koivunej
Copy link
Member

Copy link
Member

@koivunej koivunej left a comment

Choose a reason for hiding this comment

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

Looking very good.

@arpad-m
Copy link
Member Author

arpad-m commented Jul 26, 2023

So so ok, we use OnceLock where possible and OnceCell where we need fallible init and it seems that is still waiting on decisions around Try on std.

Yeah, in the future we might have to switch some to OnceCell from tokio because that one also supports async init.

Personally I'm also good with consistently using OnceCell from once_cell, shrug.

@koivunej
Copy link
Member

koivunej commented Jul 26, 2023

Personally I'm also good with consistently using OnceCell from once_cell, shrug.

Your call. I was hopeful of us no longer needing to use once_cell at all, but perhaps moving from it doesn't make sense if we cannot drop it as a dependency (which will probably never happen because of MSRV of our dependencies, realized only now). So maybe it was just a silly idea from me.

@arpad-m
Copy link
Member Author

arpad-m commented Jul 26, 2023

@koivunej yeah it will be years until the ecosystem switches off once_cell, even if the standard library catches up. I think it's best to not do the change for now, it's easy to do later in the future. We gain consistency and if someone does a refactor that requires a fallible init, they don't have to switch to once_cell.

@arpad-m arpad-m merged commit 5705413 into main Jul 26, 2023
29 checks passed
@arpad-m arpad-m deleted the arpad/once_cell branch July 26, 2023 15:20
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.

3 participants