dsmiley commented on code in PR #2621:
URL: https://github.com/apache/solr/pull/2621#discussion_r1709937216
##########
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
Review Comment:
You could have returned the IOException and let the caller throw it.
##########
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:
A matter of taste but I would prefer we/programmers use Optionals
conservatively -- when it's clearly a help. They are verbose; should have been
built into the language like Kotlin. In particular, I suggest we not use
Optional as a parameter; only return them if we use them.
See https://www.baeldung.com/java-optional #15 (and I've seen this advice
elsewhere too)
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
