Skip to content

[FIXED] Default to allowing binary stream snapshots#7479

Merged
neilalexander merged 1 commit intomainfrom
maurice/binary-cap
Oct 28, 2025
Merged

[FIXED] Default to allowing binary stream snapshots#7479
neilalexander merged 1 commit intomainfrom
maurice/binary-cap

Conversation

@MauriceVanVeen
Copy link
Copy Markdown
Member

When routes to other servers are established binarySnapshots is initially set to false indicating it is not supported. Then once a STATSZ is received binarySnapshots will be updated to true.

Whether this server supports binary snapshots is determined by looping over the peers and checking if other servers don't support it:

if sir, ok := s.nodeToInfo.Load(p.ID); ok && sir != nil && !sir.(nodeInfo).binarySnapshots

However, there's an issue here as the server will shortly think binarySnapshots isn't supported and go back to the legacy snapshot which contains interior deletes as a []uint64 which will be disastrous if a stream has a huge amount of interior deletes. Since binary stream snapshots are supported since 2.10.0, we can simply flip the binarySnapshots default from false to true.

Alternative to #7476

Signed-off-by: Maurice van Veen github@mauricevanveen.com

Signed-off-by: Maurice van Veen <github@mauricevanveen.com>
@MauriceVanVeen MauriceVanVeen requested a review from a team as a code owner October 28, 2025 09:47
Copy link
Copy Markdown
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

Principally OK with me, with the caveat that this will make one-shot downgrades to 2.9.x or earlier potentially unsafe. Will await @derekcollison's opinion too.

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

@wallyqs
Copy link
Copy Markdown
Member

wallyqs commented Oct 28, 2025

include warning from @alexbozhenko branch? a4a17f4

Copy link
Copy Markdown
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM

For downgrade to 2.9, what would actually happen? Can we rebuild and ask for a new one in old style if we do not know what it is?

@MauriceVanVeen
Copy link
Copy Markdown
Member Author

For downgrade to 2.9, what would actually happen? Can we rebuild and ask for a new one in old style if we do not know what it is?

Binary snapshots have been introduced since 2.10.0, so this PR doesn't change what would happen if binary snapshots exist, but:
For example, when going from 2.10.0 that contains binary snapshots to 2.9.x, I think the 2.9 server would already not understand the binary snapshot and break?

@neilalexander neilalexander merged commit dd29850 into main Oct 28, 2025
89 of 92 checks passed
@neilalexander neilalexander deleted the maurice/binary-cap branch October 28, 2025 17:07
neilalexander added a commit that referenced this pull request Oct 28, 2025
Includes the following:

- #7380
- #7384
- #7385
- #7388
- #7395
- #7400
- #7399
- #7401
- #7402
- #7423
- #7424
- #7411
- #7428
- #7429
- #7431
- #7435
- #7433
- #7443
- #7455
- #7465
- #7466
- #7460
- #7484
- #7479

Signed-off-by: Neil Twigg <neil@nats.io>
neilalexander added a commit that referenced this pull request Oct 30, 2025
Includes the following:

- #7435
- #7433
- #7436
- #7443
- #7440
- #7444
- #7452
- #7455
- #7458
- #7465
- #7466
- #7474
- #7469
- #7460
- #7449
- #7484
- #7479
- #7486
- #7495
- #7482
- #7496

Signed-off-by: Neil Twigg <neil@nats.io>
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.

4 participants