feat: modify recon store to support event validation concepts #473
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
dav1do
requested review from
a team,
AaronGoldman and
nathanielc
as code owners
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
dav1do
commented
Aug 12, 2024
stbrody
reviewed
Aug 12, 2024
…d/invalid item context
we also explicitly hangup if the store says something is invalid
this probalby doesn't hurt to include, but we're particular about using our AHash so we'll leave it explicit for now
- rename InsertBatch -> InsertResult - use for loop in pending_cache instead of while loop - consistent Self::Key usage in trait
- recon pending tracking simplified to just retry every batch since we should get items close together - fixes a bug where any event in an api batch that was invalid could have crashed the entire batch (now only that one gets an error) - TODO: still need to modify the service to have a better validation hook that keeps track of the data we need to return so we don't have to iterate back through or lose info along the way
dav1do
force-pushed
the
feat/aes-304-recon-unprocessable
branch
from
f1ab99a
to
cd34094
Compare
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
stbrody
reviewed
Aug 14, 2024
dav1do
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
Merged
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.
The recon store returns a new struct that includes information about the store's willingness to process the data. It includes invalid items (e.g. something that made no sense) and the count of "pending" items (i.e. something that can't be interpreted without another event. For now, recon simply logs the count of pending/invalid items in prometheus and we close the conversation if a peer is sending us invalid items. There are also some changes to the service layer to support returning this information to the API and recon.
We don't track the pending items yet, we intend to have the event service keep a map of sorts from
{neededCid: [events]}and then it can process them when discovered. This may cause odd behavior if recon isn't getting ranges for discovered keys, so we may need to push this information back up eventually. I initially added this tracking to recon but we pretty much just send them back every time unless we communicate more information about what is needed to recon. Since we don't really care to actively seek this out yet, having the store just manage this pending set will be simpler, and if recon not getting in sync is causing problems we'll have to fix that, but it can be tested with the next phase of event validation where we actually run into these.Previously we just threw an error when an event can't be parsed, which has been modified to return invalid and errors will be reserved for real problems like an issue talking to the storage system. There is now a struct with some recon config injected (batch size, remote peer). I don't really like this, but didn't want to update the recon or role traits to require this. We can do that (or something else) if it's preferred, for now this was simple.
This targets #470, if it's easier I can merge them into one but there are already a decent number of changes in both. This does rework the API changed in the previous one, so consolidating might make sense.