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

chore: remove panic and continue with old config on incremental update failure #4108

Closed
wants to merge 2 commits into from

Conversation

Sidddddarth
Copy link
Member

@Sidddddarth Sidddddarth commented Nov 9, 2023

Description

incremental update failure causes a panic. Removing that panic.

Linear Ticket

Slack thread for context

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

Copy link

codecov bot commented Nov 9, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (4b5e926) 71.92% compared to head (f850c3f) 71.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4108      +/-   ##
==========================================
+ Coverage   71.92%   71.96%   +0.04%     
==========================================
  Files         373      373              
  Lines       54933    54942       +9     
==========================================
+ Hits        39511    39541      +30     
+ Misses      13103    13086      -17     
+ Partials     2319     2315       -4     
Files Coverage Δ
backend-config/namespace_config.go 78.26% <100.00%> (+4.97%) ⬆️
backend-config/backend-config.go 83.00% <0.00%> (-1.06%) ⬇️

... and 7 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

workspaceID, req,
))
nc.logger.Errorw(
"workspace was not updated but was not present in previous config",
Copy link
Contributor

Choose a reason for hiding this comment

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

we can just add an alert on this log right?

Comment on lines +181 to +183
if errors.Is(err, ErrIncrementalUpdateFailed) {
stats.Default.NewStat("config_backend.ErrIncrementalUpdateFailed", stats.CountType).Increment()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need another stat?

Comment on lines +148 to +149
// reset state here
nc.lastUpdatedAt = time.Time{}
Copy link
Contributor

Choose a reason for hiding this comment

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

namespaceConfig#Get can react on the ErrIncrementalUpdateFailed by resetting the lastUpdatedAt and calling getFromAPI again. This way we won't have to wait for the next poll to happen

@atzoum
Copy link
Contributor

atzoum commented Nov 9, 2023

If we want this to be released as a patch release ASAP it needs to target 1.16.x, otherwise it will be included in the upcoming 1.17.x release

@Sidddddarth
Copy link
Member Author

If we want this to be released as a patch release ASAP it needs to target 1.16.x, otherwise it will be included in the upcoming 1.17.x release

My bad
Opened here

@Sidddddarth Sidddddarth closed this Nov 9, 2023
@Sidddddarth Sidddddarth deleted the chore.noPanicOnIncrementalUpdateFailure branch November 9, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants