Skip to content

Dry up inputstream to bytesreference #43675

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

Conversation

original-brownbear
Copy link
Member

  • Dry up spots where we use the same pattern to get from an InputStream to a BytesReferences

* Dry up spots where we use the same pattern to get from an InputStream to a BytesReferences
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

Nice clean up, I left some comments

throw e;
}
// EMPTY is safe here because RepositoryData#fromXContent calls namedObject
final BytesReference out = Streams.readFully(blobContainer().readBlob(snapshotsIndexBlobName));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to fully read the blob here. Instead we could pass the InputStream directly to the XContentParser and let the blob containter bufferizes the reads?

repositoryData = repositoryData.incompatibleSnapshotsFromXContent(parser);
}
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE,
Streams.readFully(blobContainer().readBlob(INCOMPATIBLE_SNAPSHOTS_BLOB)), XContentType.JSON)) {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Streams.copy(blob, out);
return Numbers.bytesToLong(out.bytes().toBytesRef());
}
return Numbers.bytesToLong(Streams.readFully(blobContainer().readBlob(INDEX_LATEST_BLOB)).toBytesRef());
Copy link
Member

Choose a reason for hiding this comment

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

We expect this file to be very small so using Streams.readFully()can makes sense here.

out.bytes(), XContentType.JSON)) {
repositoryData = RepositoryData.snapshotsFromXContent(parser, latestGen);
}
try (XContentParser parser = XContentHelper.createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@original-brownbear original-brownbear requested a review from tlrx July 2, 2019 15:26
@original-brownbear
Copy link
Member Author

Thanks @tlrx I moved to directly deserializing in the spots you pointed out now :)

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

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

LGTM

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/packaging-sample

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/2

@original-brownbear
Copy link
Member Author

thanks @tlrx !

(packaging sample is just red because of the build stats cluster being down -> merging)

@original-brownbear original-brownbear merged commit d7cc9de into elastic:master Jul 2, 2019
@original-brownbear original-brownbear deleted the dry-up-inputstream-to-bytesreference branch July 2, 2019 18:33
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Jul 9, 2019
* Dry up Reading InputStream to BytesReference

* Dry up spots where we use the same pattern to get from an InputStream to a BytesReferences
original-brownbear added a commit that referenced this pull request Jul 9, 2019
* Dry up Reading InputStream to BytesReference
* Dry up spots where we use the same pattern to get from an InputStream to a BytesReferences
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants