[IMPROVED] Check stream state performance #5963
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
…uld check all msgs from the streams first sequence through ack floor and up to delivered. We do this to make sure our ack state is correct. In cases where there were alot of messages still in the stream due to offline or clow consumers, this could be a heavy load on a server. This improvement uses LoadNextMsg() to efficiently skip ahead and we now remember our checked floor and do not repeat checks for messages below our check floor on subsequent runs. Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
We used to check at 5s then every 30s. However we already checked once the log replay was complete. Now just check every ~2.5 minutes. Signed-off-by: Derek Collison <[email protected]>
Signed-off-by: Derek Collison <[email protected]>
| psi.fblk = i | ||
| // We only require read lock here as that is desirable, | ||
| // so we need to do this in a go routine to acquire write lock. | ||
| go func() { |
There was a problem hiding this comment.
Do we need to guard against accidentally creating multiple of these goroutines for the same store?
There was a problem hiding this comment.
I don't think so, but was thinking same thing.
We essentially have three options.
- Promote to write lock at call sites, but I want LoadNextMsg() to be able to operate in parallel.
- Do not do any fixups in this function to stale fblks.
- The proposal above (we could modify it to funnel through a single Go routine, but should not be common)
There was a problem hiding this comment.
I'm wondering if a simple CompareAndSwap and a deferred clear would be enough just to ensure only one fixup runs at a time for a given store. I'd be worried that we could end up with multiple of these doing the same work at the same time, which could compound the issue.
There was a problem hiding this comment.
Could be for a different set of PSIM entries though so a simple boolean state would not suffice IMO.
There was a problem hiding this comment.
Good point, we could go with this for now and keep an eye on the goroutines. It may not be an issue.
There was a problem hiding this comment.
I think worse case is we could duplicate work but not invalidate state. Hence the checks to only move it forward.
neilalexander
pushed a commit
that referenced
this pull request
Oct 8, 2024
When checking interest state for interest or workqueue streams, we would check all msgs from the streams first sequence through ack floor and up to delivered. We do this to make sure our ack state is correct. In cases where there were alot of messages still in the stream due to offline or slow consumers, this could be a heavy load on a server. This improvement uses LoadNextMsg() to efficiently skip ahead and we now remember our checked floor and do not repeat checks for messages below our check floor on subsequent runs. This change also highlighted a datarace in filestore that is fixed here as well. Signed-off-by: Derek Collison <[email protected]> --------- Signed-off-by: Derek Collison <[email protected]>
neilalexander
added a commit
that referenced
this pull request
Oct 9, 2024
wallyqs
pushed a commit
that referenced
this pull request
Oct 13, 2024
When checking interest state for interest or workqueue streams, we would check all msgs from the streams first sequence through ack floor and up to delivered. We do this to make sure our ack state is correct. In cases where there were alot of messages still in the stream due to offline or slow consumers, this could be a heavy load on a server. This improvement uses LoadNextMsg() to efficiently skip ahead and we now remember our checked floor and do not repeat checks for messages below our check floor on subsequent runs. This change also highlighted a datarace in filestore that is fixed here as well. Signed-off-by: Derek Collison <[email protected]> --------- Signed-off-by: Derek Collison <[email protected]>
MauriceVanVeen
added a commit
that referenced
this pull request
Dec 17, 2024
Signed-off-by: Maurice van Veen <[email protected]>
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.
When checking interest state for interest or workqueue streams, we would check all msgs from the streams first sequence through ack floor and up to delivered.
We do this to make sure our ack state is correct. In cases where there were alot of messages still in the stream due to offline or slow consumers, this could be a heavy load on a server.
This improvement uses LoadNextMsg() to efficiently skip ahead and we now remember our checked floor and do not repeat checks for messages below our check floor on subsequent runs.
This change also highlighted a datarace in filestore that is fixed here as well.
Signed-off-by: Derek Collison [email protected]