Add retry for DB connection#208
Conversation
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
==========================================
+ Coverage 86.14% 86.73% +0.59%
==========================================
Files 23 23
Lines 1335 1342 +7
==========================================
+ Hits 1150 1164 +14
+ Misses 135 127 -8
- Partials 50 51 +1
Continue to review full report at Codecov.
|
| return err | ||
| }, | ||
| retry.Attempts(config.Config.DBConnectionRetryAttempts), | ||
| retry.Delay(config.Config.DBConnectionRetryDelay), |
There was a problem hiding this comment.
btw, the default retryType is backoff.
https://github.com/avast/retry-go/blob/master/retry.go#L80-L87
There was a problem hiding this comment.
Awesome. By default 10 retries gives us 25.5 seconds of reconnect time. If we change the default attempts to 9, our retry time comes down to 12.7 seconds. That seems more like a happy medium between long enough and not too long to me. What do you think?
| // DBConnectionDebug controls whether to show the database connection debugging logs | ||
| // warning: it may log the credentials to the stdout | ||
| DBConnectionDebug bool `env:"FLAGR_DB_DBCONNECTION_DEBUG" envDefault:"true"` | ||
| // DBConnectionRetryAttempts controls how we are going to retry on db connection when start the flagr server |
There was a problem hiding this comment.
a note to myself, we probably need to document in the env.go, what are the minimal env variables do we need for a new setup.
There was a problem hiding this comment.
This looks great! Thanks for looking into this so quickly. Only thing I noticed is that the retry-go package was never added to the Gopkg.toml file — I believe it should be there. Retry-go has has a few breaking changes through major releases, so it would be good to protect against that.
| return err | ||
| }, | ||
| retry.Attempts(config.Config.DBConnectionRetryAttempts), | ||
| retry.Delay(config.Config.DBConnectionRetryDelay), |
There was a problem hiding this comment.
Awesome. By default 10 retries gives us 25.5 seconds of reconnect time. If we change the default attempts to 9, our retry time comes down to 12.7 seconds. That seems more like a happy medium between long enough and not too long to me. What do you think?
zhouzhuojie
force-pushed
the
zz/add-db-connection-retry
branch
from
19aada2 to
71cb04a
Compare
zhouzhuojie
force-pushed
the
zz/add-db-connection-retry
branch
from
71cb04a to
30100e0
Compare
3 participants
Description
Fix #207
Motivation and Context
Add retries so that the DB connection is more reliable when we start the flagr server.
How Has This Been Tested?
Test locally with sqlite3. Will go through integration tests
Types of changes
Checklist: