Clean up global variables in safekeeper code #8200
Labels
a/tech_debt
Area: related to tech debt
c/storage/safekeeper
Component: storage: safekeeper
m/good_first_issue
Moment: when doing your first Neon contributions
Comments
petuhovskiy
added
c/storage/safekeeper
|
@petuhovskiy can I take up this issue ? |
@fowlerlee |
github-merge-queue bot
pushed a commit
that referenced
this issue
Dec 9, 2024
Hello! I was interested in potentially making some contributions to Neon and looking through the issue backlog I found [8200](#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](#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 public`get_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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We have at least several places in the code where global variables are used:
GlobalTimelines::<funcname>to lookup timelinesstatic TIMELINES_STATEstores all timelines as a global variablestatic REMOTE_STORAGEstores config for remote storage (e.g. S3)To resolve the issue, we should replace
GlobalTimelines::calls withArc<GlobalTimelines>struct. It will contain the hashmap of all timelines (the same wayTIMELINES_STATEis working now). It should be passed around to the code that needs access to all timelines, instead of everything relying on global access throughGlobalTimelines.GlobalTimelines::callsstatic TIMELINES_STATEREMOTE_STORAGEtoo should be passed around as aArc<RemoteStorage>. Each task that requires access to remote storage should getArc<RemoteStorage>as a dependency during initialization.static REMOTE_STORAGEIf some background task requires access to all global state (remote storage, timeline, config), we can bundle them together inside
Arc<SafekeeperApp>, which is easier to carry around.Also, currently
struct GlobalTimelinesStatecontains two fields that are part of the global state:Currently
confis required in many places and usually it just copied from global state on demand. The config is loaded once on startup and never changes. This is wasteful, we should replace it withArc<SafeKeeperConf>and always carryArc<..>instead ofSafeKeeperConf.SafeKeeperConfusages withArc<SafeKeeperConf>everywhereWhy?
To improve testing, to allow to run more than one safekeeper instance in a single process.
To simplify creating new dependencies without introducing global variables and special getters.
The text was updated successfully, but these errors were encountered: