pkg/stash: add Etcd as a SingleFlight backend #1070
Conversation
marwan-at-work
force-pushed
the
withetcd
branch
from
e1fe764 to
2c55d0d
Compare
Codecov Report
@@ Coverage Diff @@
## master #1070 +/- ##
==========================================
- Coverage 54.83% 53.29% -1.54%
==========================================
Files 79 80 +1
Lines 2639 2719 +80
==========================================
+ Hits 1447 1449 +2
- Misses 1073 1151 +78
Partials 119 119
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
@marwan-at-work not only is this awesome on its own, but it'll be a great blueprint go build other drivers after this goes in. thank you 🎉
I left some nits, small requests and questions in here
docker-compose.yml
Outdated
| volumes: | ||
| etcd0: | ||
| etcd1: | ||
| etcd2: |
There was a problem hiding this comment.
@marwan-at-work nit: can you add a newline at the end of the file?
pkg/config/singleflight.go
Outdated
| // that helps Athens connect to the | ||
| // Etcd backends. | ||
| type Etcd struct { | ||
| Endpoints string |
There was a problem hiding this comment.
Can you explicitly name this and the above struct field with tags? I think explicit is better than implicit here
There was a problem hiding this comment.
@arschles +1 on this field, missed the env override. But the above struct field does not need it just like the StorageConfig
| }) | ||
| } | ||
|
|
||
| err = eg.Wait() |
There was a problem hiding this comment.
is there a way to do this with a timeout? it'd be nice to avoid having integration tests hang forever if there's something wrong with an etcd server running in docker or something else. maybe this can be done by creating eg with a context that times out? I don't remember if that's how it works...
There was a problem hiding this comment.
@arschles nice suggestion, I'll add a timeout on the context of the Stash function, should work fine.
marwan-at-work
force-pushed
the
withetcd
branch
from
2c55d0d to
281dfa9
Compare
| @@ -39,6 +39,8 @@ type Config struct { | |||
| HGRCPath string `envconfig:"ATHENS_HGRC_PATH"` | |||
| TLSCertFile string `envconfig:"ATHENS_TLSCERT_FILE"` | |||
| TLSKeyFile string `envconfig:"ATHENS_TLSKEY_FILE"` | |||
| SingleFlightType string `envconfig:"ATHENS_SINGLE_FLIGHT_TYPE"` | |||
This is a more polished version of my previous PR (#933)
It fixes the review comments, merge conflicts, and is also no longer WIP so it's ready to merge.
Apologies if I missed any of the previous comments, feel free to call them out.
Fixes #760