chore(redis): Make pipeline configurable #101
Conversation
Current coverage is 41.48%@@ master #101 diff @@
==========================================
Files 10 10
Lines 321 323 +2
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 132 134 +2
Misses 169 169
Partials 20 20
|
storage/redis_adapter.go
Outdated
| } | ||
|
|
||
| func newMessagePipeliner(bufferSize int, redisClient *r.Client, errCh chan error) *messagePipeliner { | ||
| func newMessagePipeliner(bufferSize int, redisClient *r.Client, timeoutSeconds int, errCh chan error) *messagePipeliner { |
There was a problem hiding this comment.
can you just pass a time.Duration here
There was a problem hiding this comment.
So... I did try having a time.Duration in the redisConfig type, but found that envconfig couldn't handle that. So, barring that, the only way to pass a time.Duration to this function would be for any caller to convert the int to a time.Duration before calling. Somehow I'm not sure if that's actually better, but I am certain I could be convinced.
There was a problem hiding this comment.
yea, fair enough. I was suggesting converting it to a time.Duration at the config struct, and passing time.Durations down the stack from there. thoughts?
There was a problem hiding this comment.
Ah. Yes... I can manually convert if in the parseConfig() function. I didn't think of that before.
|
#101 (comment) is not blocking this PR. LGTM2 |
|
@arschles I amended the PR as per your suggestion about passing around |
| DB int `envconfig:"DEIS_LOGGER_REDIS_DB" default:"0"` | ||
| PipelineLength int `envconfig:"DEIS_LOGGER_REDIS_PIPELINE_LENGTH" default:"50"` | ||
| PipelineTimeoutSeconds int `envconfig:"DEIS_LOGGER_REDIS_PIPELINE_TIMEOUT_SECONDS" default:"1"` | ||
| PipelineTimeout time.Duration |
There was a problem hiding this comment.
might want to leave this out of the struct and make it a function on the struct. past versions (at least) of envconfig would try to parse this
There was a problem hiding this comment.
Seems to be ok with the current version. I'm going to leave it be for now.
|
@krancour cool - still LGTM |
At @jchauncey's suggestion, I am making Redis pipelines more configurable in terms of their length and in terms of how long we will queue up messages before executing a pipeline that has not yet reached the configured length. (This is to avoid a large delay in messages reaching Redis when log volumes are low.)
This PR does not change the default values.