-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Enable the errcheck
linter
#1170
base: master
Are you sure you want to change the base?
Conversation
errcheck
linter
this.collectGeneralThrottleMetrics() | ||
firstThrottlingCollected <- true | ||
for { | ||
if err := this.collectGeneralThrottleMetrics(); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're slowly replicating the entire ticker loop that follows, perhaps we should refactor initiateThrottlerCollection
to something along the lines of the below - what do you think?
(notifyFirstThrottlingCollected
could probably be better named!)
func (this *Throttler) initiateThrottlerCollection(notifyFirstThrottlingCollected chan<- bool) {
go this.collectReplicationLag(notifyFirstThrottlingCollected)
go this.collectControlReplicasLag()
go this.collectThrottleHTTPStatus(notifyFirstThrottlingCollected)
firstMetricsCollected := false
go func() {
ticker := time.NewTicker(time.Second)
defer ticker.Stop()
for range ticker.C {
if atomic.LoadInt64(&this.finishedMigrating) > 0 {
return
}
err := this.collectGeneralThrottleMetrics()
if err != nil {
this.migrationContext.Log.Errorf("Failed to collect throttler metrics: %+v", err)
} else if !firstMetricsCollected {
notifyFirstThrottlingCollected <- true
firstMetricsCollected = true
}
}
}()
}
@@ -378,7 +384,9 @@ func (this *Applier) WriteChangelog(hint, value string) (string, error) { | |||
} | |||
|
|||
func (this *Applier) WriteAndLogChangelog(hint, value string) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we never actually use the string
return value from the WriteAndLogChangelog
, WriteChangelogState
, and WriteChangelog
functions - what do you think about getting rid of it and just returning error
?
Description
This PR enables the
errcheck
linter in the projectgolangci-lint
config:Also a lot of missing error handling was added for problems (or
// nolint:errcheck
flags for some false-positives). For the most part I have avoided introducing new scenarios where we fail/exit the migration, so in many cases I just-added alog.Errorf(...)
without modifying thereturn
script/cibuild
returns with no formatting errors, build errors or unit test errors.