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

JAVA-3033 - Consider handling IOException in a manner consistent with other exceptions in CloudConfigFactory.fetchProxyMetadata() #1609

Closed
wants to merge 2 commits into from

Conversation

tongjixianing
Copy link

No description provided.

@tongjixianing
Copy link
Author

This PR originally is about handling the IOE from HttpsURLConnection.getInputStream() call in fetchProxyMetaData method which was triggered by 401 request error. During the Unit test, we also found it also needs to handle the FileNotFoundException triggered by 404 error code when the metadata request URL is not correct or the service cannot be located.

… other exceptions in CloudConfigFactory.fetchProxyMetadata()
@hhughes hhughes force-pushed the java3033 branch 2 times, most recently from fc66bca to 296ad86 Compare July 27, 2023 22:35
@hhughes
Copy link
Contributor

hhughes commented Jul 27, 2023

Taking over this PR so we can close out this feature branch.

  • rebased with latest 4.x
  • moved FNF handling to earlier catch block so it doesn't get grabbed by the clause catching it's superclass IOE
  • Use exact exception message matching in the unit test to make sure that the correct ISEs are thrown
  • Added test for 502 bad gateway which was the error I got from astra when fiddling with the /metadata path
  • Removed wildcard exceptions

- Move FNF handling to before IOE (FNF is subclass of IOE)
- Update tests to assert exact message strings
- add test for bad gateway (502) from /metadata
- remove wildcard exceptions
} catch (FileNotFoundException e) {
throw new IllegalStateException(
"Metadata service request path not found. Please make sure the metadata service url is correct",
e);
Copy link
Contributor

Choose a reason for hiding this comment

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

This change relies on undocumented behaviour (HttpsURLConnection throwing FNFE in this case). We should either find a more conventional way to note this case or else remove the special logging for this specific code path.

throw new IllegalStateException(
"Unable to read data from cloud metadata service. Please make sure your cluster is not parked or terminated",
ioe);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like for more targeted catch blocks here. As it is the exceptions indicated here could be thrown by either HttpsURLConneciton.getInputStream(), the InputStreamReader constructor or the BufferedReader constructor.

@absurdfarce absurdfarce deleted the java3033 branch August 21, 2023 05:51
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