[MCLEAN-124] Leverage Files.delete(Path) API to provide more accurate reason in case of failure #84
Conversation
| } catch (InterruptedException e2) { | ||
| exception.addSuppressed(e2); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
I'm confused. If the thread has already thrown an interrupted exception, why are you interrupting it again? Are these two different threads?
There was a problem hiding this comment.
As we don't re-throw InterruptedException here we should set interrupted flag agin
There was a problem hiding this comment.
I'm not sure that's what that says. It suggests resetting the interrupted flag if you don;t handle the exception, but here it's handled by throwing an IOException.
And now that I look at this, is that even right? We're sleeping between retries, the sleep is interrupted, so we cancel all the retries and throw an IOException instead of returning. I guess that makes sense, but at this point the interrupt has been handled so it doesn't seem like the current thread should still have its interrupt flag set.
| try { | ||
| Files.deleteIfExists(file); | ||
| } catch (IOException e) { | ||
| return e; |
There was a problem hiding this comment.
This is an unidiomatic pattern for Java. I would expect this to simply throw the exception and have that case handled in a catch block.
There was a problem hiding this comment.
Maybe can be designed in better way, but please consider that it is moved code from 3.x branch to master.
slawekjaranowski
force-pushed
the
cp-60
branch
from
09408d0 to
7d9928c
Compare
| } catch (InterruptedException e2) { | ||
| exception.addSuppressed(e2); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
I'm not sure that's what that says. It suggests resetting the interrupted flag if you don;t handle the exception, but here it's handled by throwing an IOException.
And now that I look at this, is that even right? We're sleeping between retries, the sleep is interrupted, so we cancel all the retries and throw an IOException instead of returning. I guess that makes sense, but at this point the interrupt has been handled so it doesn't seem like the current thread should still have its interrupt flag set.
… reason in case of failure
slawekjaranowski
force-pushed
the
cp-60
branch
from
7d9928c to
0e8342e
Compare
|
Resolve #180 |
Following this checklist to help us incorporate your
contribution quickly and easily:
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
[MCLEAN-XXX] - Fixes bug in ApproximateQuantiles,where you replace
MCLEAN-XXXwith the appropriate JIRA issue. Best practiceis to use the JIRA issue title in the pull request title and in the first line of the
commit message.
mvn clean verifyto make sure basic checks pass. A more thorough check willbe performed on your pull request automatically.
mvn -Prun-its clean verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.