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

feat(storage): thread new storage reference through all read requests #2570

Merged
merged 5 commits into from
Jan 8, 2024

Conversation

GeorgeMac
Copy link
Member

@GeorgeMac GeorgeMac commented Dec 20, 2023

Fixes: FLI-724

This is a combination refactor and addition of a new reference field on all read requests.

In order to thread this everywhere with an empty default, I packaged up many of the storage method call arguments into structures. In particular, I created a few common structures.

NamespaceRequest is used for operations which only need a namespace:

// NamespaceRequest is used to identify a request predicated by both a revision and a namespace.
// This is used to identify namespaces and list resources directly beneath them (e.g. flags and segments).
type NamespaceRequest struct {
ReferenceRequest
key string
}

ResourceRequest is used for any operation which is scoped to some resource (e.g. scoped by flag or segment):

// ResourceRequest is used to identify a request predicated by revision, namespace and a key.
// This is used for core resources (e.g. flag and segment) as well as to list sub-resources (e.g. rules and constraints).
type ResourceRequest struct {
NamespaceRequest
Key string
}

Each of these types embed a reference which can be empty (zero state).
In a subsequent PR I will start to depend upon these references.
However, the server layer has been updated in this change to start threading these references from the RPC layer into the storage layer.

@GeorgeMac GeorgeMac force-pushed the gm/add-read-reference branch 6 times, most recently from d1ef1a5 to e1e653a Compare December 22, 2023 16:12
@GeorgeMac GeorgeMac marked this pull request as ready for review December 22, 2023 16:17
@GeorgeMac GeorgeMac requested a review from a team as a code owner December 22, 2023 16:17
Copy link

codecov bot commented Dec 22, 2023

Codecov Report

Attention: 25 lines in your changes are missing coverage. Please review.

Comparison is base (5aa70ec) 71.57% compared to head (2f33b1d) 71.37%.

Files Patch % Lines
internal/storage/fs/snapshot.go 82.14% 9 Missing and 1 partial ⚠️
internal/server/evaluator.go 50.00% 8 Missing ⚠️
internal/storage/fs/store.go 88.70% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2570      +/-   ##
==========================================
- Coverage   71.57%   71.37%   -0.21%     
==========================================
  Files          84       84              
  Lines        8085     8013      -72     
==========================================
- Hits         5787     5719      -68     
- Misses       1947     1949       +2     
+ Partials      351      345       -6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

amazing refactor!!

@GeorgeMac GeorgeMac force-pushed the gm/add-read-reference branch from e1e653a to 0456eb8 Compare January 2, 2024 10:21
@GeorgeMac GeorgeMac requested a review from markphelps January 2, 2024 11:20
@GeorgeMac
Copy link
Member Author

@markphelps I just renamed a bunch of these new things and added some additional doc comments.
I mostly renamed things from "predicate" to "request" as I felt it was too unecessarilly fancy talk 🎩 .

@GeorgeMac
Copy link
Member Author

Also, this last commit allows the existing fs stores to skip taking a reference and ignoring it for now.
Instead, there is an adaptor that does the dropping reference part and log if its non-empty.
The Git and OCI stores will ultimately implement the ReferencedSnapshotStore interface and support multiple references that way. But the others will warn if you try to access no default references through the adaptor.

@GeorgeMac GeorgeMac force-pushed the gm/add-read-reference branch from a936863 to 7bec446 Compare January 2, 2024 15:07
markphelps
markphelps previously approved these changes Jan 2, 2024
Copy link
Collaborator

@markphelps markphelps left a comment

Choose a reason for hiding this comment

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

wicked

@GeorgeMac GeorgeMac force-pushed the gm/add-read-reference branch 2 times, most recently from 63d1eca to 0036b52 Compare January 7, 2024 10:20
@GeorgeMac GeorgeMac requested a review from markphelps January 7, 2024 10:20
@GeorgeMac
Copy link
Member Author

Last pass @markphelps

I rebased and fixed this up after the the GCS changes and the cache changes 👍

refactor(storage): thread reference on storage request structs

refactor(storage): rename ListPredicates to PageParameter

refactor(storage): rename NewListRequest to ListWithOptions

refactor(storage): rename List to ListWithParameters

refactor(storage): rename ReferencePredicate to ReferenceRequest

refactor(storage): rename NamespacePredicate to NamespaceRequest

refactor(storage): rename ResourcePredicate to ResourceRequest

refactor(storage): rename IDPredicate to IDRequest

chore(rpc/flipt): proto-gen-go comment version updates

refactor(storage): abstract reference threading behind new ReferencedSnapshotStore interface
@GeorgeMac GeorgeMac force-pushed the gm/add-read-reference branch from 0036b52 to cbf2c53 Compare January 7, 2024 10:26
@GeorgeMac GeorgeMac dismissed markphelps’s stale review January 7, 2024 19:01

I've updated the PR

@GeorgeMac GeorgeMac enabled auto-merge (squash) January 7, 2024 19:02
Copy link
Contributor

@yquansah yquansah left a comment

Choose a reason for hiding this comment

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

This looks good to me! Nice work.

p := IDRequest{ID: id}
containers.ApplyAll(&p.ReferenceRequest, opts...)
return p
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice abstractions!

@GeorgeMac GeorgeMac merged commit 3659f93 into main Jan 8, 2024
34 of 35 checks passed
@GeorgeMac GeorgeMac deleted the gm/add-read-reference branch January 8, 2024 13:40
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