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

Add Blob Download Retries to GCS Repository #52479

Merged
merged 3 commits into from
Feb 19, 2020

Conversation

original-brownbear
Copy link
Member

Exactly as #46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS.

Closes #52319

Exactly as elastic#46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS.

Closes elastic#52319
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

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, thanks Armin

private InputStream openStream() throws IOException {
try {
final ReadChannel readChannel = SocketAccess.doPrivilegedIOException(() -> client.reader(blobId));
readChannel.seek(currentOffset);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe only seek if currentOffset > 0L?

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't have any IO impact in the SDK (it just sets an long field that is then used when firing off the actual request) so I figured why do 3 lines instead of 1 unless we have to? :)

Copy link
Member

Choose a reason for hiding this comment

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

I figured why do 3 lines instead of 1 unless we have to? :)

I see things differently, why always seek to the beginning when most of the time it's not necessary? :)

Anyway, it's nitpicking so do as you prefer :)

@original-brownbear
Copy link
Member Author

Thanks Tanguy!

@original-brownbear original-brownbear merged commit c665cf0 into elastic:master Feb 19, 2020
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Feb 19, 2020
* Add Blob Download Retries to GCS Repository

Exactly as elastic#46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS.

Closes elastic#52319
@original-brownbear original-brownbear deleted the 52319 branch February 19, 2020 16:29
sbourke pushed a commit to sbourke/elasticsearch that referenced this pull request Feb 19, 2020
* Add Blob Download Retries to GCS Repository

Exactly as elastic#46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS.

Closes elastic#52319
original-brownbear added a commit that referenced this pull request Feb 19, 2020
* Add Blob Download Retries to GCS Repository

Exactly as #46589 (and kept as close to it as possible code wise so we can dry things up in a follow-up potentially) but for GCS.

Closes #52319
@original-brownbear original-brownbear restored the 52319 branch August 6, 2020 18:24
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.

GCS SDK does not resume downloads that fail part-way through
4 participants