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

Simplify XContent detection. #14472

Merged
merged 1 commit into from Nov 9, 2015

Conversation

Projects
None yet
4 participants
@jpountz
Copy link
Contributor

commented Nov 3, 2015

Currently we duplicate the logic for BytesReferences and InputStreams.

@bleskes

View changes

core/src/main/java/org/elasticsearch/common/xcontent/XContentFactory.java Outdated
if (Character.isWhitespace(val) == false) {
final byte[] firstBytes = new byte[GUESS_HEADER_LENGTH];
int read = 0;
while (read < firstBytes.length) {

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 3, 2015

Member

can we use org.elasticsearch.common.io.Streams#readFully(java.io.InputStream, byte[]) ?

This comment has been minimized.

Copy link
@jpountz

jpountz Nov 3, 2015

Author Contributor

good point!

This comment has been minimized.

Copy link
@bleskes

bleskes Nov 3, 2015

Member

I wonder if we should this api to the forbidden api pointing people at the utilities... (like we did in the write variants).

@bleskes

This comment has been minimized.

Copy link
Member

commented Nov 3, 2015

+1 on share the code. I wonder if we should flip this around and use the stream input as the basic implementation and use org.elasticsearch.common.bytes.BytesReference#streamInput to call it from the bytes reference variant. It's just slighty more efficient as it doesn't read ahead.

@jpountz

This comment has been minimized.

Copy link
Contributor Author

commented Nov 3, 2015

I wondered about the same but using the stream input version as a basic implementation had the downside of being a bit harder to read (because it had to check every read against -1 while the BytesReference version can just check the length once) as well as require the stream input returned by every BytesReference to support mark/reset.

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Nov 3, 2015

lets not over-design this - simplicity is king here

@bleskes

This comment has been minimized.

Copy link
Member

commented Nov 3, 2015

yeah, I hear you. I'm not sure if it's that's easier to read but we're splitting hairs :) one up side of keeping the stream input variant as the basis is that it might be easier to convert org.elasticsearch.common.xcontent.XContentFactory#xContentType(java.lang.CharSequence) as well (wrapping the char sequence with a InputStreams)

@bleskes

This comment has been minimized.

Copy link
Member

commented Nov 3, 2015

so bottom line - up to you :)

Simplify XContent detection.
Currently we duplicate the logic for BytesReferences and InputStreams.

@jpountz jpountz force-pushed the jpountz:fix/simplify_xcontent_detection branch to c350a3d Nov 9, 2015

jpountz added a commit that referenced this pull request Nov 9, 2015

@jpountz jpountz merged commit f400f58 into elastic:master Nov 9, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jpountz jpountz deleted the jpountz:fix/simplify_xcontent_detection branch Nov 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.