gerlowskija commented on code in PR #2621:
URL: https://github.com/apache/solr/pull/2621#discussion_r1719161093
##########
solr/core/src/java/org/apache/solr/handler/IndexFetcher.java:
##########
@@ -1988,16 +1989,38 @@ private FastInputStream getStream() throws IOException {
req.setBasePath(leaderBaseUrl);
if (useExternalCompression) req.addHeader("Accept-Encoding", "gzip");
response = solrClient.request(req, leaderCoreName);
+ final var responseStatus = (Integer) response.get("responseStatus");
is = (InputStream) response.get("stream");
+
+ if (responseStatus != 200) {
+ final var errorMsg =
+ String.format(
+ Locale.ROOT,
+ "Unexpected status code [%d] when downloading file [%s].",
+ responseStatus,
+ fileName);
+ closeStreamAndThrowIOE(is, errorMsg, Optional.empty());
+ }
+
if (useInternalCompression) {
is = new InflaterInputStream(is);
}
return new FastInputStream(is);
} catch (Exception e) {
- // close stream on error
- IOUtils.closeQuietly(is);
- throw new IOException("Could not download file '" + fileName + "'", e);
+ closeStreamAndThrowIOE(is, "Could not download file '" + fileName +
"'", Optional.of(e));
+ }
+
+ // Unreachable b/c closeStreamAndThrowIOE will always throw
+ return null;
+ }
+
+ private void closeStreamAndThrowIOE(
+ InputStream is, String exceptionMessage, Optional<Exception> e) throws
IOException {
Review Comment:
I've seen the critique of `Optionals` before, but I'll admit it doesn't
makes a lot of sense to me.
**If** your only goal in using an `Optional` is to avoid a null-check,
**and** you want to be absolutely rigid on sanity-checking your inputs...then
sure, I guess taking an `Optional` parameter doesn't help you.
But it has other benefits - among them its self-documenting nature. It
signals to readers unequivocally that this parameter doesn't need a value,
strictly speaking. IMO `Optional` used in this way is like a better version of
the `@Nullable` annotation - "better" because `@Nullable` only serves as a
reminder for those reading the method signature itself, whereas `Optional` in
its role as conduit to the underlying value manages to reinforce this reminder
throughout the method implementation.
That said - I understand I'm a minority opinion here, so I'm happy to defer
to the Java community's consensus on this. I'll try to remember this and not
introduce further usage in this vein.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
