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

Fix Bug in Azure Repo Exception Handling #47968

Conversation

original-brownbear
Copy link
Member

We were incorrectly handling IOExceptions thrown by
the InputStream side of the upload operation, resulting
in a ClassCastException as we expected to never get
IOException from the Azure SDK code but we do in practice.
This PR also sets an assertion on markSupported for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on IOException in the InputStream.

We were incorrectly handling `IOExceptions` thrown by
the `InputStream` side of the upload operation, resulting
in a `ClassCastException` as we expected to never get
`IOException` from the Azure SDK code but we do in practice.
This PR also sets an assertion on `markSupported` for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on `IOException` in the `InputStream`.
@elasticmachine
Copy link
Collaborator

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

@@ -294,6 +296,44 @@ public void testWriteLargeBlob() throws Exception {
assertThat(blocks.isEmpty(), is(true));
}

public void testRetryUntilFail() throws IOException {
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 test is admittedly a little random/weird, but I figured it captured the initial issue that showed this bug to some degree at least.

if (cause instanceof IOException) {
throw (IOException) e.getCause();
} else {
throw new IOException(cause);
Copy link
Contributor

Choose a reason for hiding this comment

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

It feels wrong to wrap this as an IOException. Can we just use Throwables.rethrow(cause) 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'd rather not tbh. We could wrap in a different exception but Throwables.rethrow itself just casts to RunTimeException doesn't it? In the end if the SDK ever changes or we missed some checked exception in our code, we'll get the same bug we started from here again won't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

but Throwables.rethrow itself just casts to RunTimeException doesn't it

no it does not

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, Java works in mysterious ways. Sorry for the noise, my bad seems it works just fine :)
=> used it all the way now.

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/2 (random ML thing)

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/packaging-sample (Vagrant ...)

@original-brownbear
Copy link
Member Author

Jenkins run elasticsearch-ci/2 (random ML thing again )

Copy link
Contributor

@ywelsch ywelsch 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

Thank Yannick + Tanguy!

I'm wondering, should this go to 6.8.x as well? It's kind of a cosmetic issue I suppose, but it makes it hard for users to fix/identify problems with Azure ...

@original-brownbear original-brownbear merged commit 58c62c5 into elastic:master Oct 15, 2019
@original-brownbear original-brownbear deleted the fix-azure-exeception-handling branch October 15, 2019 01:40
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Oct 15, 2019
We were incorrectly handling `IOExceptions` thrown by
the `InputStream` side of the upload operation, resulting
in a `ClassCastException` as we expected to never get
`IOException` from the Azure SDK code but we do in practice.
This PR also sets an assertion on `markSupported` for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on `IOException` in the `InputStream`.
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Oct 15, 2019
We were incorrectly handling `IOExceptions` thrown by
the `InputStream` side of the upload operation, resulting
in a `ClassCastException` as we expected to never get
`IOException` from the Azure SDK code but we do in practice.
This PR also sets an assertion on `markSupported` for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on `IOException` in the `InputStream`.
@ywelsch
Copy link
Contributor

ywelsch commented Oct 15, 2019

I'm wondering, should this go to 6.8.x as well?

yes

original-brownbear added a commit that referenced this pull request Oct 15, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We were incorrectly handling `IOExceptions` thrown by
the `InputStream` side of the upload operation, resulting
in a `ClassCastException` as we expected to never get
`IOException` from the Azure SDK code but we do in practice.
This PR also sets an assertion on `markSupported` for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on `IOException` in the `InputStream`.
original-brownbear added a commit that referenced this pull request Oct 15, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We were incorrectly handling `IOExceptions` thrown by
the `InputStream` side of the upload operation, resulting
in a `ClassCastException` as we expected to never get
`IOException` from the Azure SDK code but we do in practice.
This PR also sets an assertion on `markSupported` for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on `IOException` in the `InputStream`.
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Oct 15, 2019
We were incorrectly handling `IOExceptions` thrown by
the `InputStream` side of the upload operation, resulting
in a `ClassCastException` as we expected to never get
`IOException` from the Azure SDK code but we do in practice.
This PR also sets an assertion on `markSupported` for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on `IOException` in the `InputStream`.
original-brownbear added a commit that referenced this pull request Oct 15, 2019

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
We were incorrectly handling `IOExceptions` thrown by
the `InputStream` side of the upload operation, resulting
in a `ClassCastException` as we expected to never get
`IOException` from the Azure SDK code but we do in practice.
This PR also sets an assertion on `markSupported` for the
streams used by the SDK as adding the test for this scenario
revealed that the SDK client would retry uploads for
non-mark-supporting streams on `IOException` in the `InputStream`.
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

5 participants