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

Compact snapshot stats: Fix incorrect counting for current snapshot #481

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

lfittl
Copy link
Member

@lfittl lfittl commented Nov 29, 2023

Due to the way the code was written, we would either count the current snapshot, or emit statistics - but even if we emit statistics we still need to increment the snapshot count.

Due to the way the code was written, we would either count the current
snapshot, or emit statistics - but even if we emit statistics we still
need to increment the snapshot count.
@lfittl lfittl requested a review from keiko713 November 29, 2023 08:29
}
}
if len(details) > 0 {
logger.PrintInfo("Compact snapshots submitted: " + details)
Copy link
Member Author

Choose a reason for hiding this comment

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

Whilst looking at this I also considered whether we should reword this to "Submitted compact snapshots" (to be more consistent with the previous wording, and what's used for full snapshots), but I suspect this was intentionally worded this way in #467 since we're talking about what happened in the last minute (vs what just happened), so I think this is okay to leave worded as is.

cc @msakrejda in case you have any wording thoughts on this message.

for context, right now it is:

Compact snapshots submitted: 5 activity, 3 logs

and I'm wondering if it shouldn't be:

Submitted compact snapshots: 5 activity, 3 logs

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is how it looks like currently, so I can see that a previous wording matches to the full snapshot and might be easier to also scan through.
Alternatively, we could also update the "Submitted snapshot successfully" and make it "Full snapshot submitted", though it doesn't look good 🤔

 I [default] Compact snapshots submitted: 5 activity, 2 logs
 I [default] Compact snapshots submitted: 5 activity, 2 logs
 I [default] Submitted snapshot successfully
 I [default] Compact snapshots submitted: 5 activity, 2 logs

Copy link
Member Author

@lfittl lfittl Nov 29, 2023

Choose a reason for hiding this comment

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

If we were to change it, I'm thinking this would be most logical:

 I [default] Submitted compact snapshots successfully: 5 activity, 2 logs
 I [default] Submitted full snapshot successfully

Let me make a separate PR for this, and we can discuss there.

Copy link
Member Author

Choose a reason for hiding this comment

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

See #482

@lfittl lfittl requested a review from a team November 29, 2023 08:33
@lfittl lfittl merged commit 7b335b6 into main Nov 29, 2023
6 checks passed
@lfittl lfittl deleted the improve-compact-snapshot-stats branch November 29, 2023 08:58
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