-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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.
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
Implement overwrite in PageDoraWorker.rename() #18049
base: main
Are you sure you want to change the base?
Conversation
@Jackson-Wang-7 ask for review |
dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java
Outdated
Show resolved
Hide resolved
&& options.getS3SyntaxOptions().getOverwrite(); | ||
if (dstUfs.exists(dst)) { | ||
if (!overWrite) { | ||
throw new RuntimeException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why it should be a RuntimeException, just the FileAlreadyExitsException is enough.
dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java
Outdated
Show resolved
Hide resolved
if (dstUfs.getStatus(dst).isFile()) { | ||
dstUfs.deleteFile(dst); | ||
} else { | ||
dstUfs.deleteDirectory(dst, DeleteOptions.RECURSIVE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, we are not allowed to overwrite a non-empty directory.
@huanght1997 ptal, this PR will add a overwrite option in rename api, what's your opinion? |
@huanghua78 I remove the envelopment of |
@@ -731,35 +731,29 @@ public OpenFileHandle createFile(String path, CreateFilePOptions options) | |||
if (options.hasRecursive() && options.getRecursive()) { | |||
createOption.setCreateParent(true); | |||
} | |||
|
|||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the outer most try
statement removed?
The original code is to catch any IOException here and convert it to a RuntimeException().
Because RuntimeException is expected on gRPC client side, IIRC.
Please double check the necessity of this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alluxio.worker.grpc.DoraWorkerClientServiceHandler#rename:326
calls AlluxioRuntimeException.from(e)
. Both IOException and RuntimeException are converted to UnknownRuntimeException finally. so I think it's no need to envelop IOException as RuntimeException.
Please also check this: |
2fec0ec
to
b597c61
Compare
What changes are proposed in this pull request?
I implement overwrite in pageDoraWorker.rename()
Why are the changes needed?
there is a problem that overwrite can't be done while completing multipart upload if the object exists before. I implement overwrite in pageDoraWorker.rename() to make complete MPU behaves right.
Does this PR introduce any user facing changes?
none