Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Aug 23, 2018

What changes were proposed in this pull request?

Make sure TransportServer and SocketAuthHelper close the resources for all types of errors.

How was this patch tested?

Jenkins

try {
if (channel != null) {
long size = channel.size();
throw new IOException("Error in reading " + this + " (actual file length " + size + ")",
Copy link
Member Author

Choose a reason for hiding this comment

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

This is just thrown and then ignored. I assigned it to errorMessage so that we can see it in the error.

try {
if (is != null) {
long size = file.length();
throw new IOException("Error in reading " + this + " (actual file length " + size + ")",
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

@SparkQA
Copy link

SparkQA commented Aug 24, 2018

Test build #95184 has finished for PR 22210 at commit 6f2248d.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member Author

zsxwing commented Aug 27, 2018

cc @brkyvz

@brkyvz
Copy link
Contributor

brkyvz commented Aug 27, 2018

LGTM! Good catches

@HyukjinKwon
Copy link
Member

Seems okay to me too

@zsxwing
Copy link
Member Author

zsxwing commented Aug 28, 2018

Thanks! Merging to master.

@asfgit asfgit closed this in 592e3a4 Aug 28, 2018
@zsxwing zsxwing deleted the SPARK-25218 branch August 28, 2018 17:13
bogdanrdc pushed a commit to bogdanrdc/spark that referenced this pull request Aug 28, 2018
…nd SocketAuthHelper

## What changes were proposed in this pull request?

Make sure TransportServer and SocketAuthHelper close the resources for all types of errors.

## How was this patch tested?

Jenkins

Closes apache#22210 from zsxwing/SPARK-25218.

Authored-by: Shixiong Zhu <[email protected]>
Signed-off-by: Shixiong Zhu <[email protected]>
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.

4 participants