Skip to content

Conversation

@nik9000
Copy link
Member

@nik9000 nik9000 commented Feb 27, 2017

Adds a common base class for testing subclasses of
InternalSingleBucketAggregation. They are so similar they
call into question the utility of having all of these classes.
We maybe could just use InternalSingleBucketAggregation in
all those cases.... But for now, let's test the classes!

Relates to #22278

Adds a common base class for testing subclasses of
`InternalSingleBucketAggregation`. They are so similar they
call into question the utility of having all of these classes.
We maybe could just use `InternalSingleBucketAggregation` in
all those cases.... But for now, let's test the classes!

Relates to elastic#22278
@nik9000 nik9000 added :Analytics/Aggregations Aggregations >test Issues or PRs that are addressing/adding tests v5.4.0 v6.0.0-alpha1 labels Feb 27, 2017
@nik9000 nik9000 added the review label Feb 27, 2017
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor comment regarding overflow in document counts.
LGTM otherwise

emptyMap()));
}
// we shouldn't use the full long range here since we sum doc count on reduce, and don't want to overflow the long range there
long docCount = between(0, Integer.MAX_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment is not enough, we should protect against overflow so 100 or 200 should be enough ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docCount is a a long so it's totally fine, sorry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a funny story to tell you about labels sometime. Remind me the next time we see each other in person.

@nik9000 nik9000 merged commit a54daad into elastic:master Mar 1, 2017
@nik9000
Copy link
Member Author

nik9000 commented Mar 1, 2017

Thanks for the review @jimczi!

master: a54daad

@nik9000 nik9000 removed the v5.4.0 label Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations >test Issues or PRs that are addressing/adding tests v6.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants