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

Add comments to events based cache code #5327

Merged
merged 9 commits into from
Aug 20, 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 some comments to make the workflow more clear.
  • Update a pair of if statements to make the code easier to read.

Comment on lines -114 to +117
// the longer running transaction will be skipped over
if !a.firstEventTime.IsZero() && event.EventID != a.lastEventID+1 {
// the longer running transaction will be skipped over.
if !a.firstEventTime.IsZero() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following same style suggested by this code review comment.

Comment on lines -127 to +130
// the longer running transaction will be skipped over
if !a.firstEventTime.IsZero() && event.EventID != a.lastEventID+1 {
// the longer running transaction will be skipped over.
if !a.firstEventTime.IsZero() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Following same style suggested by this code review comment.

@MarcosDY MarcosDY added this to the 1.10.1 milestone Jul 30, 2024
@@ -188,7 +188,6 @@ func (a *attestedNodes) replayMissedEvents(ctx context.Context) {
switch status.Code(err) {
case codes.OK:
case codes.NotFound:
log.Debug("Event not yet populated in database")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do two things here:

  1. Leave a comment as to why we don't log anything (noise that was brought up)
  2. Make this a metric? That way at least we're not blind to the situation but it won't muddy our logs.

Side quesiton: Where did we land on this being downgraded to a Trace log to reduce the noise instead of getting rid of it altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @stevend-uber for the feedback. Ill work on making the metrics changes. I didnt understand the side question though.

Copy link
Contributor

Choose a reason for hiding this comment

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

For #1: Mostly why we don't do anything for case codes.NotFound:.

Upon initial reading of the code without your or my context, another contributor is likely to add a log here and we're back where we started :)

It's not blocking so I'm cool just adding it later :)

Copy link
Contributor

Choose a reason for hiding this comment

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

The logging for not found is likely to be better handled in #5341 as part of the backoff algorithm, and we intend to add a guage to track the entries in #4720. With these items, which are more specific, I'll ask that we not attempt non-comment changes in this PR, as it makes the PR lose focus and then velocity reduces.

If the two PRs mentioned above don't handle the issue, please search #5327, #5342, #4720, or #5349 to see if it is covered there, and if not, open a new PR.

@MarcosDY MarcosDY modified the milestones: 1.10.1, 1.10.2 Aug 1, 2024
@edwbuck
Copy link
Contributor

edwbuck commented Aug 16, 2024

@azdagron Please see if you can provide a second review on this item, I would review it, but I want eyes further from the solution to see if the comments read better to a wider audience.

Copy link
Member

@azdagron azdagron left a comment

Choose a reason for hiding this comment

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

Looks good. Just a few nits here and there.

faisal-memon and others added 2 commits August 19, 2024 13:39
Co-authored-by: Andrew Harding <[email protected]>
Signed-off-by: Faisal Memon <[email protected]>
@MarcosDY MarcosDY merged commit 42225bf into spiffe:main Aug 20, 2024
34 checks passed
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.

5 participants