Skip to content

Add warning about legacy snapshot#7485

Closed
alexbozhenko wants to merge 1 commit intomainfrom
add_warning
Closed

Add warning about legacy snapshot#7485
alexbozhenko wants to merge 1 commit intomainfrom
add_warning

Conversation

@alexbozhenko
Copy link
Copy Markdown
Member

Follow-up to #7479
There we fixed the default, but there is no test that we do not revert to the old behavior.
It is possible to imagine a scenario where some regression/race condition is introduced, and we fall back to legacy JSON again.

Add a warning that we should never see. And if we do see it, we would immediately know there is a problem.

Signed-off-by: Alex Bozhenko alexbozhenko@gmail.com

Signed-off-by: Alex Bozhenko <alexbozhenko@gmail.com>
@alexbozhenko alexbozhenko requested a review from a team as a code owner October 28, 2025 23:22
Copy link
Copy Markdown
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

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

LGTM, a warning here just in case would narrow down investigating this (or deleting the code for legacy snapshot)

@neilalexander
Copy link
Copy Markdown
Member

I'm not really convinced that there's much point in logging this. It isn't meaningful or actionable to users, plus it will just result in an awful lot of log spam during a one-step rolling upgrade from a very old version to a current one.

@ripienaar
Copy link
Copy Markdown
Contributor

Agree with Neil this kind of pointless log spam is already at a quite annoying level and this ads no value to end users.

@alexbozhenko
Copy link
Copy Markdown
Member Author

@neilalexander @ripienaar
We do not really test that binary snapshots are used and legacy are not, do we?
That was quite an expensive way to discover this edge case.
So I was thinking if there is a regression later, it would potentially get unnoticed until there is another OOM.
Do you have suggestions for other ways to prevent legacy snapshots from showing up again?

@neilalexander
Copy link
Copy Markdown
Member

I would prefer that we add an Antithesis assertion instead, that way it is reported by tooling designed to uncover these edge-cases instead of just creating noise in the logs.

@MauriceVanVeen
Copy link
Copy Markdown
Member

I would prefer that we add an Antithesis assertion instead, that way it is reported by tooling designed to uncover these edge-cases instead of just creating noise in the logs.

#7486

neilalexander added a commit that referenced this pull request Oct 29, 2025
…7486)

Closes #7485.

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
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.

5 participants