Skip to content

RetryingInputEntity to retry on transient errors#8923

Merged
jihoonson merged 4 commits intoapache:masterfrom
jihoonson:retry-input-entity
Nov 22, 2019
Merged

RetryingInputEntity to retry on transient errors#8923
jihoonson merged 4 commits intoapache:masterfrom
jihoonson:retry-input-entity

Conversation

@jihoonson
Copy link
Copy Markdown
Contributor

@jihoonson jihoonson commented Nov 21, 2019

Description

Reading from some inputSources such as S3 or GS should retry on transient errors.


This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.
  • added integration tests.
  • been tested in a test Druid cluster.

This change is Reviewable

Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

lgtm 👍

import java.io.InputStream;
import java.net.URI;

public class RetryingInputEntity implements InputEntity
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I changed my mind, I think this might be better as part of the InputEntity interface so we don't have to wrap RetryingInputEntity everywhere, e.g. swap the default to be for read() instead of read(long offset) and provide a default implementation for getRetryCondition that is always false?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I like this more. 👍 Will update my pr soon.

@lgtm-com
Copy link
Copy Markdown

lgtm-com Bot commented Nov 21, 2019

This pull request introduces 1 alert when merging db25fa2 into dc6178d - view on LGTM.com

new alerts:

  • 1 for Potential input resource leak

Copy link
Copy Markdown
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

much nicer imo 👍

* Directly opens an {@link InputStream} starting at the given offset on the input entity.
* This is the basic way to read the given entity.
*/
InputStream readFrom(long offset) throws IOException;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would there ever be cases where the offset type for an InputEntity might be something other than a long?

If not, can you add a comment about that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm good question. It's hard for me to imagine an offset of other types since InputEntity abstracts an byte-representable entity. I added javadoc on it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool, thanks

@jihoonson jihoonson merged commit 934547a into apache:master Nov 22, 2019
@jihoonson
Copy link
Copy Markdown
Contributor Author

@clintropolis @jon-wei thanks for the review!

jon-wei pushed a commit to jon-wei/druid that referenced this pull request Nov 26, 2019
* RetryingInputEntity to retry on transient errors

* fix some javadoc and httpEntity

* Make it interface

* Javadoc for offset
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
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.

3 participants