Add parsing for InternalBucketMetricValue #24182
Conversation
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
I left a question on the new interface, LGTM otherwise
There was a problem hiding this comment.
there was no interface before? That may complicate migration from transport client for users? or was SingleValue enough from the Java api?
There was a problem hiding this comment.
My guess is transport client users that needed only value() or getValueAsString() would cast to NumericMetricsAggregation.SingleValue, if you needed keys() you would have to cast to InternalBucketMetricValue. I think it is a kind of oversight that there is no interface, other aggregations that are the output of pipeline aggregators (like InternalSimpleValue) have an interface.
There was a problem hiding this comment.
Lets ask @colings86 if this interface was left out intentionally and whether we should add it on master/5.x so this works similar to e.g. InternalSimpleValue class / SimpleValue interface.
There was a problem hiding this comment.
I think this interface must have been left out by accident. I think it makes sense to add it especially since there is an interface for SimpleValue
There was a problem hiding this comment.
thanks @colings86, I will pull adding the interface out as a single PR against master branch then like Luca suggested.
cbuescher
force-pushed
the
addParsing-BucketMetricValue
branch
from
f121f13 to
bd0b396
Compare
There was a problem hiding this comment.
Sorry if I'm asking something thats already been discussed but why are we adding this method given we already have value() below?
There was a problem hiding this comment.
I only added this because similar single value interfaces like Sum/Min/Max etc... implement double getValue() in addition to double value(), just to stay consistend. Happy to remove this from this interface, as you said we don't really need it.
There was a problem hiding this comment.
I think this interface must have been left out by accident. I think it makes sense to add it especially since there is an interface for SimpleValue
cbuescher
force-pushed
the
addParsing-BucketMetricValue
branch
from
bd0b396 to
526b23a
Compare
| // cannot differentiate between them. Also we cannot recreate the exact String representation | ||
| assertEquals(parsed.value(), Double.NEGATIVE_INFINITY, 0); | ||
| } | ||
|
|
|
|
||
| public class ParsedBucketMetricValue extends ParsedSingleValueNumericMetricsAggregation implements BucketMetricValue { | ||
|
|
||
| private List<String> keys = new ArrayList<>(); |
There was a problem hiding this comment.
Right, thats better in this case. Thanks.
4 participants
Similar to #24085 this adds parsing for InternalBucketMetricValue. This will be needed for
the high level rest client. I also added a new BucketMetricValue interface since InternalBucketMetricValue didn't implement an interface yet so the keys() method wouldn't be accessible on the client side without casting to a concrete implementation.
PR is against the current feature branch for aggregation parsing.