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

safekeeper: use arc for global timelines and config #10051

Merged

Conversation

oldmanfleming
Copy link
Contributor

Hello! I was interested in potentially making some contributions to Neon and looking through the issue backlog I found 8200 which seemed like a good first issue to attempt to tackle. I see it was assigned a while ago so apologies if I'm stepping on any toes with this PR. I also apologize for the size of this PR. I'm not sure if there is a simple way to reduce it given the footprint of the components being changed.

Problem

This PR is attempting to address part of the problem outlined in issue 8200. Namely to remove global static usage of timeline state in favour of Arc<GlobalTimelines> and to replace wasteful clones of SafeKeeperConf with Arc<SafeKeeperConf>. I did not opt to tackle RemoteStorage in this PR to minimize the amount of changes as this PR is already quite large. I also did not opt to introduce an SafekeeperApp wrapper struct to similarly minimize changes but I can tackle either or both of these omissions in this PR if folks would like.

Summary of changes

  • Remove static usage of GlobalTimelines in favour of Arc<GlobalTimelines>
  • Wrap SafeKeeperConf in Arc to avoid wasteful clones of the underlying struct

Some additional thoughts

  • We seem to currently store SafeKeeperConf in GlobalTimelines and then expose it through a publicget_global_config function which requires locking. This seems needlessly wasteful and based on observed usage we could remove this public accessor and force consumers to acquire SafeKeeperConf through the new Arc reference.

@oldmanfleming oldmanfleming requested a review from a team as a code owner December 8, 2024 19:10
@oldmanfleming oldmanfleming requested a review from arpad-m December 8, 2024 19:10
@github-actions github-actions bot added the external A PR or Issue is created by an external user label Dec 8, 2024
Copy link
Member

@arpad-m arpad-m left a comment

Choose a reason for hiding this comment

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

LGTM, cc @arssher just to make sure

@arpad-m arpad-m added the approved-for-ci-run Changes are safe to trigger CI for the PR label Dec 9, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Dec 9, 2024
@vipvap vipvap mentioned this pull request Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

7051 tests run: 6726 passed, 0 failed, 325 skipped (full report)


Flaky tests (2)

Postgres 14

Code coverage* (full report)

  • functions: 31.4% (8332 of 26527 functions)
  • lines: 47.7% (65545 of 137449 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
c0a55ee at 2024-12-09T17:14:00.178Z :recycle:

@arpad-m
Copy link
Member

arpad-m commented Dec 9, 2024

status: blocked on #10059. @oldmanfleming after it merged, could you merge main? (better not a rebase as we merge with squash anyways)

@oldmanfleming
Copy link
Contributor Author

@arpad-m Thanks for the review! #10059 is merged and I merged main to this branch. Let me know if I messed something up with the merging

@arpad-m arpad-m added the approved-for-ci-run Changes are safe to trigger CI for the PR label Dec 9, 2024
@github-actions github-actions bot removed the approved-for-ci-run Changes are safe to trigger CI for the PR label Dec 9, 2024
@arpad-m
Copy link
Member

arpad-m commented Dec 9, 2024

@oldmanfleming thanks for merging latest main. I will give some time for @arssher and @petuhovskiy to give it a look as they are more familiar with the code than I am, but it looks good from my point of view. Definitely an improvement.

Copy link
Member

@petuhovskiy petuhovskiy left a comment

Choose a reason for hiding this comment

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

Very cool, thank you @oldmanfleming!

@arpad-m can you run the CI and merge this?

@bayandin
Copy link
Member

bayandin commented Dec 9, 2024

arpad-m can you run the CI and merge this?

The CI has passed already 🎉

@arpad-m arpad-m added this pull request to the merge queue Dec 9, 2024
Merged via the queue into neondatabase:main with commit b593e51 Dec 9, 2024
88 checks passed
@arpad-m
Copy link
Member

arpad-m commented Dec 9, 2024

Merged, thanks again.

@oldmanfleming
Copy link
Contributor Author

@arpad-m No problemo 😄

@oldmanfleming oldmanfleming deleted the 8200-cleanup-global-timelines branch December 13, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external A PR or Issue is created by an external user
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants