Skip to content
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

Check missed events before start #5289

Merged
merged 8 commits into from
Jul 24, 2024

Conversation

faisal-memon
Copy link
Contributor

Pull Request check list

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality
Events based cache

Description of change

  • Adds a new function missedStartupEvents() that checks for events below the first event we received.
  • This runs evertime the cache is updated up till 24 hours, the transaction timeout time.
  • Any events are immediately added to the cache.

Which issue this PR fixes
fixes #5151

Signed-off-by: Faisal Memon <[email protected]>
@edwbuck
Copy link
Contributor

edwbuck commented Jul 15, 2024

@azdagron This is an important PR, being the one that closes 5151 and basically fixes the last of the known bugs in the database event processing. If you are back from your travels, a quick look today might help us move it faster when we have Tuseday's meeting.

Signed-off-by: Faisal Memon <[email protected]>
Signed-off-by: Faisal Memon <[email protected]>
@edwbuck
Copy link
Contributor

edwbuck commented Jul 19, 2024

@amartinezfayo Would you please add this to the 1.10.1 release as was decided in the mainter's meeting on 7/16/2024? This is a critical bug fix.

@azdagron azdagron added this to the 1.10.1 milestone Jul 19, 2024
if err := tx.Find(&events, "id > ?", req.GreaterThanEventID).Order("id asc").Error; err != nil {
return nil, sqlError.Wrap(err)

if req.GreaterThanEventID != 0 && req.LessThanEventID != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

I suggest moving this check into buildListEventsQueryString. That body of that function only makes sense with the mutual exclusivity of these options in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@azdagron
Copy link
Member

I'd suggest a high-level explanation of the algorithm used to detect missing events and event gaps somewhere in the code. Things are a bit hard to follow and digging up the GH issue isn't convenient for folks trying to understand the code.

@azdagron azdagron merged commit 01bedb8 into spiffe:main Jul 24, 2024
33 checks passed
@faisal-memon faisal-memon deleted the events-startup-race-3 branch July 24, 2024 15:26
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.

Events based cache could lose events at startup
3 participants