Conversation
Close the error response stream and fallback input stream on success and failure. Add regression coverage for stream ownership, response formatting, and exception propagation.
| } | ||
| try (InputStream stream = errorStream) { | ||
| if (stream != null) { | ||
| error += ": " + IOUtils.toString(stream); |
Contributor
There was a problem hiding this comment.
IOUtils.toString(InputStream input) has been deprecated in the newer versions of commons-io including v2.16.1 being used in hive, since we are already updating this, can we rather use IOUtils.toString(InputStream input, Charset charset) implementation?
Replace the deprecated IOUtils.toString(InputStream) call with the Charset overload. Pass Charset.defaultCharset() to preserve the existing response decoding behavior.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Use try-with-resources in
YarnQueueHelper.handleUnexpectedStatusCode()to close the selected response stream after reading it, including when reading fails. Keep the error stream preferred over the fallback input stream.Why are the changes needed?
HIVE-29876: the method reads the response body without closing its stream.
Does this PR introduce any user-facing change?
The response selection and error message format are unchanged. A failure closing the stream now propagates as an
IOException; if reading also fails, the close exception is suppressed on the read exception.How was this patch tested?
Added nine regression tests covering both stream paths, empty and missing responses, read failures, and close failures. Seven fail against the original implementation; all nine pass with the fix.
Ran the new tests and the existing
TestTezSessionPoolon JDK 21 with Maven 3.9.11:mvn -B -ntp -Dscan=false -pl ql -am test \ -Dtest=TestYarnQueueHelper,TestTezSessionPool \ -DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=falseAll 17 tests passed, and all 21 modules in the selected reactor succeeded. Apache RAT passed. Repository Checkstyle rules reported no findings in the new test and no new findings in the modified source.
git diff --checkpassed.TestTezSessionPoollogged background-threadSessionStatenull-pointer exceptions; a separate run against the original source reproduced the same eight exceptions with all eight tests passing. The full Hive test suite was not run.