Change file exist exception to runtime exception in PagedDoraWorker #18337
Conversation
|
Probably all the behaviors that wrap IOException into RuntimeException need to be changed |
| throw new AlreadyExistsException(String.format("File %s already exists" | ||
| + "but no overwrite flag", path)); |
There was a problem hiding this comment.
You should also provide reason literally for other reviewers.
There was a problem hiding this comment.
The RuntimeException is caught by DoraWorkClientServiceHandler, which is extracted by AlluxioRuntimeException like below.
try {
...
} catch (Exception e) {
LOG.error(...);
responseObserver.onError(AlluxioRuntimeException.from(e).toGrpcStatusRuntimeException());
}However, it is just a bare java.lang.RuntimeException, so it cannot be caught in AlluxioRuntimeException.from(), so finally the FileAlreadyExistsException becomes a UnknownRuntimeException, it is weird.
There was a problem hiding this comment.
If we use AlreadyExistsException, which is a child class of AlluxioStatusException, it will be handled in here, So the problem is solved.
…m RuntimeException
| throw new AlreadyExistsException(String.format("File %s already exists" | ||
| + "but no overwrite flag", path)); |
There was a problem hiding this comment.
The RuntimeException is caught by DoraWorkClientServiceHandler, which is extracted by AlluxioRuntimeException like below.
try {
...
} catch (Exception e) {
LOG.error(...);
responseObserver.onError(AlluxioRuntimeException.from(e).toGrpcStatusRuntimeException());
}However, it is just a bare java.lang.RuntimeException, so it cannot be caught in AlluxioRuntimeException.from(), so finally the FileAlreadyExistsException becomes a UnknownRuntimeException, it is weird.
| throw new AlreadyExistsException(String.format("File %s already exists" | ||
| + "but no overwrite flag", path)); |
There was a problem hiding this comment.
If we use AlreadyExistsException, which is a child class of AlluxioStatusException, it will be handled in here, So the problem is solved.
| @@ -51,7 +51,7 @@ public void testMkdirExisting() { | |||
| assertEquals(0, mFsShell.run("mkdir", uri.toString())); | |||
| assertEquals(-1, mFsShell.run("mkdir", uri.toString())); | |||
| assertTrue( | |||
| mOutput.toString().contains("UNKNOWN: alluxio.exception.FileAlreadyExistsException")); | |||
| mOutput.toString().contains("ALREADY_EXISTS")); | |||
There was a problem hiding this comment.
If we modify the code above, the output will be something like this: ALREADY_EXISTS: /var/folders/pc/cpgks26x1lvcm03rzrmz06dh0000gn/T/junit9989567262251760026 already exists , it won't be a UNKNOWN Exception, so we have to do this change.
|
alluxio-bot, merge this please |
### What changes are proposed in this pull request? The `RuntimeException(FileAlreadyExistException)` changed to `AlreadyExistException()` in `PagedDoraWorker` ### Why are the changes needed? `RuntimeException(FileAlreadyExistException)` seems won't be caught in `DoraWorkerClientServiceHandler` when convertin it to `AlluxioRuntimeException`, which willl cause the loss status code ### Does this PR introduce any user facing changes? no pr-link: Alluxio#18337 change-id: cid-9a520c49579847bae6da21302f484ba713eeb4d9
What changes are proposed in this pull request?
The
RuntimeException(FileAlreadyExistException)changed toAlreadyExistException()inPagedDoraWorkerWhy are the changes needed?
RuntimeException(FileAlreadyExistException)seems won't be caught inDoraWorkerClientServiceHandlerwhen convertin it toAlluxioRuntimeException, which willl cause the loss status codeDoes this PR introduce any user facing changes?
no