Skip to content

Replace AggregatorParsers with namedObject#22397

Merged
nik9000 merged 4 commits intoelastic:masterfrom
nik9000:named_xcontent_aggregations
Jan 9, 2017
Merged

Replace AggregatorParsers with namedObject#22397
nik9000 merged 4 commits intoelastic:masterfrom
nik9000:named_xcontent_aggregations

Conversation

@nik9000
Copy link
Copy Markdown
Member

@nik9000 nik9000 commented Dec 31, 2016

Removes AggregatorParsers, replacing all of its functionality with
XContentParser#namedObject.

This is the third bit of payoff from #22003, one less thing to pass
around the entire application.

@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Dec 31, 2016

@imotov or @martijnvg or @javanna, could one of you have a look at this? Another namedObject side effect....

s1monw
s1monw reviewed Jan 9, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

left one comment LGTM otherwise

core/src/main/java/org/elasticsearch/search/aggregations/BaseAggregationBuilder.java Outdated
Copy link
Copy Markdown
Contributor

@s1monw s1monw Jan 9, 2017

Choose a reason for hiding this comment

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

can we maybe simplify this and just make PipelineAggregationBuilder a subclass of AggregationBuilder and remote that interface? that would be awesome

Copy link
Copy Markdown
Member Author

@nik9000 nik9000 Jan 9, 2017

Choose a reason for hiding this comment

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

Hmmm. I didn't look at that. I'll have a look! I'd love to not need the interface.

nik9000 added 4 commits January 9, 2017 13:04
@nik9000
Replace AggregatorParsers with namedObject
39ccba0 
Removes `AggregatorParsers`, replacing all of its functionality with
`XContentParser#namedObject`.

This is the third bit of payoff from elastic#22003, one less thing to pass
around the entire application.
@nik9000
Woops
d4f6ec2
@nik9000
Javadoc
5ff347a
@nik9000
Fix after rebase
6c0f4c5
@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Jan 9, 2017

@s1monw I had a look at making PipelineAggregationBuilder extends AggregationBuilder and it is possible but it'd be a mess. I found some other small cleanups I can make as a consolation prize. I'll get this rebased and in and open a PR for the consolation prize simplifications.

@nik9000 nik9000 force-pushed the named_xcontent_aggregations branch from 2b606c2 to 6c0f4c5 Compare January 9, 2017 18:59
@nik9000 nik9000 merged commit e3f77b4 into elastic:master Jan 9, 2017
@nik9000
Copy link
Copy Markdown
Member Author

nik9000 commented Jan 9, 2017 •
edited
Loading

master: e3f77b4
5.x: c392895

nik9000 added a commit that referenced this pull request Jan 9, 2017
@nik9000
Replace AggregatorParsers with namedObject (#22397)
c392895 
Removes `AggregatorParsers`, replacing all of its functionality with
`XContentParser#namedObject`.

This is the third bit of payoff from #22003, one less thing to pass
around the entire application.
@nik9000 nik9000 mentioned this pull request Jan 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@s1monw s1monw s1monw left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

>non-issue v5.2.0 v6.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nik9000 @s1monw