fix notifier handler #602
fix notifier handler #602
Conversation
* gracefully handle the case where persist messages come in for aggregations that are not/no longer active. (previously : nil pointer panic) * handle 'last' aggregation messages * crash if unknown consolidation message. because that should never happen.
mdata/aggmetric.go
Outdated
| @@ -66,6 +66,12 @@ func NewAggMetric(store Store, cachePusher cache.CachePusher, key string, retent | |||
| return &m | |||
| } | |||
|
|
|||
| func (a *AggMetric) maybeSyncChunkSaveState(ts uint32) { | |||
There was a problem hiding this comment.
i really dont like the name "maybeSyncChunkSaveState".
I would much rather you just wrapped the calls to SyncChunkSaveState in if cntMetrc != nil
There was a problem hiding this comment.
Why ? This is more concise
There was a problem hiding this comment.
"maybe*" is so cryptic. how is someone who reads this code supposed to know what maybeSyncChunkSaveState means. Under what conditions might this function save the chunkSaveState and under what conditions might it not.
There was a problem hiding this comment.
In the chunk cache i've had a similar problem with the method CacheIfHot. It doesn't always execute it's function, only if a condition is met, so I've simply put the condition into the name to make it self explanatory.
Similarly you could maybe name it syncIfObject or something like that
There was a problem hiding this comment.
I went with the maybe* because:
- I thought we could establish that as a convention (that maybeX means execute X if the receiver is not nil)
- I've used the same convention in other projects/languages
- we can also clarify the behavior in a doc comment (but that still doesn't make it obvious when reading the code from the call site)
But I don't care that much. And making it more explicit at the expense of a few sloc works for me, so i pushed that change.
aggregations that are not/no longer active.
(previously : nil pointer panic)
never happen.