Conversation
navis
force-pushed
the
deprecate-aggregator-getname
branch
from
80c5614 to
d382d32
Compare
| // "aggregate" can throw ParseExceptions if a selector expects something but gets something else. | ||
| if (reportParseExceptions) { | ||
| throw new ParseException(e, "Encountered parse error for aggregator[%s]", agg.getName()); | ||
| throw new ParseException(e, "Encountered parse error for aggregator[%s]", k); |
There was a problem hiding this comment.
yep. String k = aggFactory.getName()
|
@navis how much of a difference does this make to memory usage or batch ingestion time for your metrics in real world use case? |
|
@fjy In the worst case, 1 min changed to 10 min (with full GC on every 10 seconds). We can just change max row count for incremental index but this was easier and effective. |
|
@navis wow that's a big difference, cool |
|
will it cause compilation errors for people who have implemented their custom aggregators ? |
|
@nishantmonu51 agree on your thoughts. actually, we are using "null-returning" version rather than removing the method. would I change like that? |
|
Null returning sounds good |
|
@navis @nishantmonu51 marking it as deprecated and throwing an exception in the standard aggs sgtm. I prefer throwing an exception with a clear error message. |
|
@navis @fjy @nishantmonu51 reworked & submitted #3572 with a couple changes: (a) deprecated instead of removed the function; (b) deprecated AggregatorFactory.getAggregatorStartValue as well. Gonna close this one since that one addresses the comments so far. thanks @navis for the original patch! |
4 participants
It's not necessary but occupies memory (a string reference, about 4~8 byte). This makes substantial difference on batch ingestion time (we are using 1000+ metric).