-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[FIXED] Fixes for filestore FSS state #5616
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… and `filteredPendingLocked` Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
derekcollison
approved these changes
Jul 6, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Small comments but really good changes.
…Find` Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
Signed-off-by: Neil Twigg <neil@nats.io>
neilalexander
force-pushed
the
neil/filestorewc
branch
from
July 7, 2024 19:24
07a5f35
to
4c28118
Compare
derekcollison
approved these changes
Jul 7, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
wallyqs
changed the title
Fixes for filestore FSS state
[FIXED] Fixes for filestore FSS state
Jul 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains some bug-fixes for the filestore FSS state.
First, if the filter provided is
_EMPTY_
then we need to overwrite it to>
for the stree functions to work, but we failed to setwc
when doing so. This could affect the decision on whether to linear-scan or not. We will now setwc
too in this case.Second, if the FSS only contains a single subject then we may not correctly set
isAll
if the filter contained a wildcard, asFind
does not work with wildcards and therefore wouldn't match the single subject. Updated to useMatch
instead, so that it correctly uses wildcards.Third, replaces
Match(>)
withIter
inenforceMsgPerSubjectLimit
, not because it is behaviourally different, but because we need to walk the entire stree anyway andIter
saves CPU cycles by skipping the match step.Fourth, updates the bounds checking on various functions that use
Find
so that the logic matches theMatch
equivalents. This also matters in particular inSubjectsState
which could fail altogether when the filter was a subject literal rather than a wildcard and thepsim
was out-of-date/needing update.Fifth, in
NumPending
and infilteredPendingLocked
when matching the FSS state, if we find a partial, don't process any further matches after that. This bug was introduced in #5559 as there used to be abreak
there, whereas now theMatch
that replaces it can't be interrupted in the same way.Signed-off-by: Neil Twigg neil@nats.io