Skip to content

Store meta state in memory #836

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

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Store meta state in memory #836

merged 3 commits into from
Oct 30, 2024

Conversation

loosebazooka
Copy link
Member

Delegated targets should be loaded at target search time, so keep state in memory so we can use it as necessary.

This PR doesn't does add delegated targets support, but this is part of the process.

Delegated targets should be loaded at target search time, so
keep state in memory so we can use it as necessary.

Signed-off-by: Appu Goundan <[email protected]>
@loosebazooka loosebazooka requested a review from patflynn October 25, 2024 00:03
}

public void setRoot(Root root) throws IOException {
// call storeTrustedRoot instead of generic storeMeta because it does doesn't extra work
Copy link
Collaborator

Choose a reason for hiding this comment

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

..does doesn't..

}

public Root getRoot() throws IOException {
return findRoot().orElseThrow(() -> new IllegalStateException("No cached root to load"));
Copy link
Collaborator

@patflynn patflynn Oct 26, 2024

Choose a reason for hiding this comment

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

maybe you can provide a little more context here (in the exception message) as to why this would happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you really only call "getRoot" if you know you should have a root cached somewhere. But I guess I can add context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess from the point of the view of the user, if this blows up, what most likely happened, how can they fix it?

Copy link
Member Author

Choose a reason for hiding this comment

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

cool

return Optional.ofNullable(root);
}

public void setTimestamp(Timestamp timestamp) throws IOException {
Copy link
Collaborator

Choose a reason for hiding this comment

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

not tempted to factor out the set/find/get logic?

Copy link
Member Author

@loosebazooka loosebazooka Oct 26, 2024

Choose a reason for hiding this comment

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

I was tempted but it meant generifying some of the getters and setters. I can do that

@@ -78,6 +83,7 @@ public class Updater {
this.localStore = localStore;
this.metaFetcher = metaFetcher;
this.targetFetcher = targetFetcher;
this.trustedMeta = TrustedMeta.newTrustedMeta(localStore);
Copy link
Collaborator

Choose a reason for hiding this comment

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

since it's a read-through cache, could it be simpler to just invalidate it on update or something rather than having the updater have to know about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh like if the root updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah probably

Signed-off-by: Appu Goundan <[email protected]>
@loosebazooka
Copy link
Member Author

alright, reworked this whole thing


/**
* Generic method to store one of the {@link SignedTufMeta} resources in the local tuf store.
* Generic method to store one of the {@link SignedTufMeta} resources in the local tuf store. Do
* not use for Root role, use {@link #setRoot(Root)} instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if you do use it for Root?

Copy link
Member Author

Choose a reason for hiding this comment

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

then you don't get N.root persisted (this is functionality that was already in there). I've just formalized it.

/** An in memory cache that will pass through to a provided local tuf store. */
public class PassthroughCacheMetaStore implements MetaReader, MetaStore {
private final MetaStore localStore;
private final Map<String, SignedTufMeta<? extends TufMeta>> cache;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Guava has a Cache you can pass a read-through function to 🤔. I'm way out of date though so don't hesitate to tell me to GTFO

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's a bit heavyweight, has eviction and what not. This is not a space limited cache.

@loosebazooka loosebazooka force-pushed the store-state-in-updater branch from 888afee to 4c741ca Compare October 30, 2024 17:01
Signed-off-by: Appu Goundan <[email protected]>
@loosebazooka loosebazooka force-pushed the store-state-in-updater branch from 4c741ca to 33482eb Compare October 30, 2024 17:02
@loosebazooka loosebazooka merged commit 8e91247 into main Oct 30, 2024
21 checks passed
@loosebazooka loosebazooka deleted the store-state-in-updater branch October 30, 2024 17:42
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