Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicated buildAsBytes and corresponding toString methods #11063

Closed
wants to merge 2 commits into from

Conversation

javanna
Copy link
Member

@javanna javanna commented May 8, 2015

We have some builders, specifically query builders, SearchSourceBuilder, QuerySourceBuilder and SuggestBuilder, that implement ToXContent and also allow to build their content as bytes by simply creating a BytesReference that holds their json (or yaml etc.) content (buildAsBytes methods). They can also print out their content through toString. Made sure that those common methods are in one single place and reused where needed.

Also, merged QueryBuilder and BaseQueryBuilder and made QueryBuilder an abstract class instead of an interface.

/**
* Base class for {@link ToXContent} implementation that also support conversion to {@link BytesReference} for serialization purposes
*/
public abstract class ToXContentToBytes implements ToXContent {
Copy link
Member Author

Choose a reason for hiding this comment

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

if you hadn't noticed.... naming is hard... can somebody find a better name for this class please? :)

Copy link
Member

Choose a reason for hiding this comment

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

Why isn't it just AbstractToXContent?

Copy link
Member Author

Choose a reason for hiding this comment

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

because only a small subset of ToXContent implementers will actually need this, good point though I can rename

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine either way, agreed the name is not obvious here...

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 left ToXContentToBytes for now, wasn't a big fan of the Abstract prefix in this context. BTW this stuff will go away once we are done with the query refactoring. Parsing on the coordinating node will mean streaming intermediate query objects to the data nodes, no json streamed as raw bytes anymore, hence no need for buildAsBytes anymore :)

@javanna
Copy link
Member Author

javanna commented May 13, 2015

@jpountz can you have a look please?

@jpountz
Copy link
Contributor

jpountz commented May 13, 2015

LGTM

We parse the rewrite field in FuzzyQueryParser but we don't allow to set it via FuzzyQueryBuilder for our java api users. Added missing field and setter.

Closes elastic#11130
Closes elastic#11139
…ethods

We have some builders, specifically query builders, `SearchSourceBuilder`, `QuerySourceBuilder` and `SuggestBuilder`, that implement `ToXContent` and also allow to build their content as bytes by simply creating a `BytesReference` that holds their json (or yaml etc.) content (`buildAsBytes` methods). They can also print out their content through `toString`. Made sure that those common methods are in one single place and reused where needed.

Also, merged `QueryBuilder` and `BaseQueryBuilder` and made `QueryBuilder` an abstract class instead of an interface.
@javanna javanna force-pushed the enhancement/build_as_bytes branch from 5ac7f66 to d4405ac Compare May 13, 2015 14:00
@javanna javanna closed this in add18a5 May 13, 2015
@kevinkluge kevinkluge removed the review label May 13, 2015
@clintongormley clintongormley changed the title Java api: remove duplicated buildAsBytes and corresponding toString methods Remove duplicated buildAsBytes and corresponding toString methods Jun 7, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants