Conversation
colmsnowplow
requested review from
Josh (jbeemster) and
Mikhail (oo-sleeper)
| // this provides natural fairness over time while controlling peak memory usage. | ||
| // Example: Setting to 20 limits memory to ~20 * 10k records * avg_record_size | ||
| maxConcurrentShards int | ||
| metricsConfig MetricsConfig |
There was a problem hiding this comment.
Looks like it's here and it's in each stats receiver implementation. How is this field used here in top-level config?
There was a problem hiding this comment.
It's not, it's just passed down - I just felt that it should be a configuration of the app rather than buried in the stats receiver. Perhaps that's not the right pattern though
There was a problem hiding this comment.
I can't see how it's passed down, if we remove this field from here, everything still works, right?
I think if we have this field here, we should do enable/disable check layer above, sth like:
if k.config.metricsConfig == nil || k.config.metricsConfig.EnableEventToClient {
k.config.stats.EventToClient(*record.record.ApproximateArrivalTimestamp, record.retrievedAt)
} in kinsumer.go, not in statsd implemenation.
There was a problem hiding this comment.
Oooh I see what you mean, apologies! Yeah I agree. And actually, I dislike both 😂
Prepping a follow-up PR to hopefully do something cleaner thanks for flagging!
3 participants
This PR:
Josh (@jbeemster) as an FYI - not essential to get your review on it