Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change file exist exception to runtime exception in PagedDoraWorker #18337

Merged
merged 5 commits into from
Nov 9, 2023
+8 −9
Merged

Change file exist exception to runtime exception in PagedDoraWorker #18337

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce9de27
change exception of file exist
voddle Oct 30, 2023
0e8b9b7
format
voddle Oct 30, 2023
5019f47
throw IOE directly
voddle Oct 30, 2023
9fdcf24
adapt new exception
voddle Oct 30, 2023
966eedb
AlluxioRuntimeException cannot extract FileAlreadyExistsException fro…
YichuanSun Nov 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@
import alluxio.conf.Configuration;
import alluxio.conf.PropertyKey;
import alluxio.exception.AccessControlException;
import alluxio.exception.FileAlreadyExistsException;
import alluxio.exception.runtime.AlluxioRuntimeException;
import alluxio.exception.runtime.FailedPreconditionRuntimeException;
import alluxio.exception.runtime.UnavailableRuntimeException;
import alluxio.exception.status.AlreadyExistsException;
import alluxio.exception.status.FailedPreconditionException;
import alluxio.grpc.Command;
import alluxio.grpc.CommandType;
Expand Down Expand Up @@ -961,9 +961,8 @@ public OpenFileHandle createFile(String path, CreateFilePOptions options)
boolean overWrite = options.hasOverwrite() ? options.getOverwrite() : false;
boolean exists = ufs.exists(path);
if (!overWrite && exists) {
throw new RuntimeException(
new FileAlreadyExistsException(
String.format("File %s already exists but no overwrite flag", path)));
throw new AlreadyExistsException(String.format("File %s already exists"
+ "but no overwrite flag", path));
Comment on lines +964 to +965
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also provide reason literally for other reviewers.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we use AlreadyExistsException, which is a child class of AlluxioStatusException, it will be handled in here, So the problem is solved.

} else if (overWrite) {
// client is going to overwrite this file. We need to invalidate the cached meta and data.
mMetaManager.removeFromMetaStore(path);
Expand Down Expand Up @@ -1091,11 +1090,11 @@ public void createDirectory(String path, CreateDirectoryPOptions options)
mMetaManager.loadFromUfs(path);
mMetaManager.invalidateListingCacheOfParent(path);
if (!success) {
throw new RuntimeException(
new FileAlreadyExistsException(String.format("%s already exists", path)));
throw new AlreadyExistsException(String.format("%s already exists", path));
}
} catch (IOException e) {
throw new RuntimeException(e);
// IOE would be caught by AlluxioRuntimeException
throw e;
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...egration/src/test/java/alluxio/client/cli/fs/command/DoraMkdirCommandIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

@Test
Expand All @@ -76,7 +76,7 @@ public void testMkdirPathWithWhiteSpaces() throws Exception {
public void testMkdirInvalidPath() {
assertEquals(-1, mFsShell.run("mkdir", ""));
assertTrue(
mOutput.toString().contains("UNKNOWN: alluxio.exception.FileAlreadyExistsException"));
mOutput.toString().contains("ALREADY_EXISTS"));
}

@Test
Expand Down