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

Improve error messages when URI points to a file that doesn't exist #12490

Conversation

tejaswini-imply
Copy link
Member

@tejaswini-imply tejaswini-imply commented Apr 28, 2022

Description

GCS

Before

java.lang.RuntimeException: com.google.api.client.googleapis.json.GoogleJsonResponseException: 404 Not Found\nNo such object: gs_ingestion_test/wikipedia-2016-06-27-smpled.json

After

java.lang.RuntimeException: Error occured while trying to read uri: gs://gs_ingestion_test/wikipedia-2016-06-27-smpled.json"
...
...
Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 404 Not Found
No such object: gs_ingestion_test/wikipedia-2016-06-27-smpled.json

S3

Before

java.io.IOException: com.amazonaws.services.s3.model.AmazonS3Exception: The specified key does not exist.

After

java.lang.RuntimeException: Error occured while trying to read uri: s3://s3-ingestion-test/wikipedia-2016-06-27-smpled.json"
...
...
java.io.IOException: com.amazonaws.services.s3.model.AmazonS3Exception: The specified key does not exist.

Fixes the error message thrown with input uri when IOException while converting InputEntity to CloseableIterator<InputRow>


Key changed/added classes in this PR
  • InputEntityIteratingReader

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, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@abhishekagarwal87
Copy link
Contributor

the title is not clear. does this PR fix a NPE? if the URI is wrong, what kind of exception is thrown before and what exception will be thrown after this change?
Also what does an incorrect URI mean here? Does it mean that there is no file corresponding to that path?

@tejaswini-imply tejaswini-imply changed the title Fixes unclear RuntimeException error message thrown if URI path incorrect during ingestion Improve error messages when URI points to a file that doesn't exist Apr 28, 2022
@abhishekagarwal87
Copy link
Contributor

Thank you @tejaswini-imply. These errors are much better. Travis is likely failing because of a lack of code coverage around your change. can you also add a unit test for your change? Your changes LGTM otherwise.

Copy link
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

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

LGTM!

+1 after CI passes.

@@ -95,4 +97,33 @@ public void test() throws IOException
Assert.assertEquals(numFiles, i);
}
}

@Test
public void testIncorrectURI() throws IOException, URISyntaxException
Copy link
Contributor

Choose a reason for hiding this comment

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

does this test issue an http request to HTTP://test/path? can we use a mock entity that throws an exception instead?

@abhishekagarwal87 abhishekagarwal87 merged commit 1d1f53e into apache:master May 1, 2022
@abhishekagarwal87
Copy link
Contributor

Thank you @tejaswini-imply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants