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

Jump to bottom

Skip copy if needed #17804

Merged
merged 1 commit into from
Aug 16, 2023
Merged

Skip copy if needed #17804

merged 1 commit into from
Aug 16, 2023

Conversation

ssyssy
Copy link
Contributor

@ssyssy ssyssy commented Jul 19, 2023 •
edited
Loading

What changes are proposed in this pull request?

Skip copy process if validation fails.
Also adding more info to the progress report.

Why are the changes needed?

Feature request.

Does this PR introduce any user facing changes?

na

jja725
jja725 reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@jja725 jja725 left a comment

Choose a reason for hiding this comment

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

we would like to send a skip status to master side so we can skip the counting in the job progress and handle it properly

@ssyssy
Copy link
Contributor Author

ssyssy commented Jul 19, 2023

we would like to send a skip status to master side so we can skip the counting in the job progress and handle it properly

I see.

@ssyssy ssyssy force-pushed the copySkip branch 4 times, most recently from ec18e28 to cefc19c Compare July 25, 2023 01:46
@ssyssy ssyssy requested a review from jja725 July 25, 2023 02:18
@ssyssy
Copy link
Contributor Author

ssyssy commented Jul 25, 2023

@jja725 Tests all completed.

jja725
jja725 reviewed Jul 26, 2023
View reviewed changes
Copy link
Contributor

@jja725 jja725 left a comment

Choose a reason for hiding this comment

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

thanks for the updates, have a rough pass

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java Outdated
@@ -545,14 +546,24 @@ public ListenableFuture<List<RouteFailure>> copy(List<Route> routes, UfsReadOpti
AuthenticatedClientUser.set(readOptions.getUser());
}
checkCopyPermission(route.getSrc(), route.getDst());
try {
ValidateHandler.validate(route, writeOptions, srcFs, dstFs);
} catch (Throwable t) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I literally don't think any exception here would indicate a skip status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change it to catch only the exception which indicates the skip status.

dora/core/server/master/src/main/java/alluxio/master/job/CopyJob.java Show resolved Hide resolved
@ssyssy ssyssy force-pushed the copySkip branch 2 times, most recently from 064ebc1 to 5d41eb1 Compare July 26, 2023 21:30
@ssyssy ssyssy requested a review from jja725 July 26, 2023 21:31
yuzhu
yuzhu requested changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@yuzhu yuzhu left a comment

Choose a reason for hiding this comment

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

some high level comments after a quick pass @ssyssy

dora/core/server/worker/src/main/java/alluxio/worker/dora/PagedDoraWorker.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/main/java/alluxio/worker/task/ValidateHandler.java Outdated
* @param srcFs the source file system
* @param dstFs the destination file system
*/
public static void validate(Route route, WriteOptions writeOptions,
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of using exception to indicate error, we should use return value to indicate failed validation. Exception handling has much more overhead than a simple return value and makes code hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ic

@ssyssy ssyssy requested a review from yuzhu July 27, 2023 01:26
@ssyssy ssyssy force-pushed the copySkip branch 5 times, most recently from 9b78288 to 2ed4307 Compare July 31, 2023 22:48
@ssyssy ssyssy requested a review from yuzhu July 31, 2023 23:28
@ssyssy ssyssy force-pushed the copySkip branch 2 times, most recently from 2d4096b to 1a97286 Compare August 2, 2023 02:28
@ssyssy
Copy link
Contributor Author

ssyssy commented Aug 3, 2023

@yuzhu @jja725 Please take a look.

jja725
jja725 requested changes Aug 3, 2023
View reviewed changes
Copy link
Contributor

@jja725 jja725 left a comment

Choose a reason for hiding this comment

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

We are on the right path, leave some comments

dora/core/server/master/src/main/java/alluxio/master/job/AbstractJob.java Show resolved Hide resolved
dora/core/server/master/src/main/java/alluxio/master/job/CopyJob.java Outdated Show resolved Hide resolved
dora/core/server/master/src/main/java/alluxio/master/job/CopyJob.java Outdated Show resolved Hide resolved
dora/core/server/master/src/main/java/alluxio/master/job/CopyJob.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/dora/PagedDoraWorkerTest.java
@@ -289,6 +289,60 @@ public void testSingleFileMove() throws IOException, ExecutionException, Interru
}
}

@Test
public void testSingleFileCopySkip() throws IOException, ExecutionException,
Copy link
Contributor

Choose a reason for hiding this comment

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

do you mind adding some tests regarding copying a folder with different scenarios, thanks!

dora/core/server/worker/src/test/java/alluxio/worker/dora/PagedDoraWorkerTest.java
@@ -376,7 +429,6 @@ public void testFolderWithFileMove()
Assert.assertTrue(b.isDirectory());
Assert.assertTrue(dstD.exists());
Assert.assertTrue(dstD.isDirectory());
assertFalse(a.exists());
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused, if a still exists meaning we fail to delete a, then why we do not get any exceptions?

Copy link
Contributor Author

@ssyssy ssyssy Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like there is a bug here. I don't know why there isn't any exceptions. It shows that it succeeds to delete, but the directory is still there.

@ssyssy ssyssy force-pushed the copySkip branch 6 times, most recently from 1f12280 to 1d16d47 Compare August 10, 2023 19:04
@ssyssy ssyssy requested a review from jja725 August 10, 2023 19:06
@ssyssy ssyssy force-pushed the copySkip branch 3 times, most recently from 63dc5b0 to 3ab33ad Compare August 10, 2023 19:39
jja725
jja725 reviewed Aug 14, 2023
View reviewed changes
Copy link
Contributor

@jja725 jja725 left a comment

Choose a reason for hiding this comment

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

Almost there

dora/core/server/worker/src/test/java/alluxio/worker/dora/PagedDoraWorkerTest.java Outdated Show resolved Hide resolved
dora/core/server/worker/src/test/java/alluxio/worker/dora/PagedDoraWorkerTest.java Outdated Show resolved Hide resolved
@ssyssy ssyssy requested a review from jja725 August 16, 2023 03:26
@jja725 jja725 added the type-feature This issue is a feature request label Aug 16, 2023
@jja725
Copy link
Contributor

jja725 commented Aug 16, 2023

alluxio-bot, merge this please

@alluxio-bot alluxio-bot merged commit a43e252 into main Aug 16, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jja725 jja725 jja725 approved these changes

@yuzhu yuzhu Awaiting requested review from yuzhu

Assignees
No one assigned
Labels
type-feature This issue is a feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ssyssy @jja725 @yuzhu @alluxio-bot