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

Ignore corrupt status files and use default. #1164

Conversation

timbru
Copy link
Contributor

@timbru timbru commented Nov 28, 2023

No description provided.

@timbru timbru requested a review from ximon18 November 28, 2023 09:15
@@ -96,7 +96,10 @@ impl StatusStore {
/// so any missing, corrupted, or no longer supported data format - can be ignored.
/// It will get updated with new status values as Krill is running.
fn load_full_status(&self, ca: &CaHandle) -> KrillResult<()> {
let repo: RepoStatus = self.store.get(&Self::repo_status_key(ca))?.unwrap_or_default();
let repo: RepoStatus = match self.store.get(&Self::repo_status_key(ca)) {
Copy link
Member

Choose a reason for hiding this comment

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

I can live with this pattern though it gets repetitive given that you do it more than once.

An alternative syntax for this could be (but still requires that the pattern be repeated on each use):

let repo: RepoStatus = self.store.get(&Self::repo_status_key(ca))
    .unwrap_or(None)
    .unwrap_or_default();

It might be better to just add a get_or_default() fn to the store and use that.

Copy link
Member

Choose a reason for hiding this comment

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

Wow even uglier would be calling unwrap_or_default() twice in a chain... ;-)

Copy link
Member

Choose a reason for hiding this comment

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

I wonder, is there an issue here when there is nothing wrong with the data on disk or in whatever backend KV store, but there's just a temporary issue accessing it? Then you'd proceed anyway ignoring the good but temporarily unavailable state rather than reporting the access issue - would it be good to at least warn that the underlying "execute()" call failed?

Copy link
Member

Choose a reason for hiding this comment

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

I suppose if there is a problem accessing the underlying store then failure is going to happen soon anyway...

@timbru
Copy link
Contributor Author

timbru commented Nov 30, 2023

Commit cherry-picked into PR #1167. Will close this, and merge that.

@timbru timbru closed this Nov 30, 2023
@ximon18 ximon18 deleted the ignore-corrupt-status-files branch January 9, 2024 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants