Skip to content

Conversation

@colings86
Copy link
Contributor

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

Is this new or code that was moved? I was looking for where this came from but couldn't find anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is new but reflects the same output as BucketSelectorBuilder provides

@cbuescher
Copy link
Member

Looks good to me, left one question and a general remark concerning the way reading/writing objects is spread across abstract classes and implementations, but this is something than can be revisited later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add javadocs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do. This is actually the same change as made in #15014 where I have already added JavaDocs. I will make sure that one is merged first and the JavaDoc is retained

Copy link
Contributor

Choose a reason for hiding this comment

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

++

@jpountz
Copy link
Contributor

jpountz commented Dec 2, 2015

LGTM

@colings86 colings86 force-pushed the feature/aggs-refactoring branch from cd3937d to f386661 Compare December 2, 2015 12:32
@colings86 colings86 merged commit 728d607 into elastic:feature/aggs-refactoring Dec 7, 2015
@colings86 colings86 deleted the refactor/bucketSelectorAgg branch December 7, 2015 11:00
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Search Refactoring labels Feb 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Analytics/Aggregations Aggregations :Search/Search Search-related issues that do not fall into other categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants