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

[ALLUXIO-3063] Support recursive ufs sync #6699

Merged
merged 5 commits into from
Jan 5, 2018
Merged

[ALLUXIO-3063] Support recursive ufs sync #6699

merged 5 commits into from
Jan 5, 2018

Conversation

gpang
Copy link
Contributor

@gpang gpang commented Jan 4, 2018

@gpang
Copy link
Contributor Author

gpang commented Jan 4, 2018

@calvinjia @aaudiber

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/18203/
Test PASSed.

@alluxio-bot
Copy link
Contributor

Automated checks report:

  • Valid pull request title: PASS
  • Contains link to JIRA ticket: PASS
  • Commits associated with Github account: PASS

All checks passed!

calvinjia
calvinjia requested changes Jan 4, 2018
View reviewed changes
Copy link
Contributor

@calvinjia calvinjia left a comment

Choose a reason for hiding this comment

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

When is the ufs sync not binary (ie. recursive or not recursive) but rather recursive up to a certain depth? I'm wondering if we can change the numerical value to a boolean.

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
listStatus = ufs.listStatus(ufsUri.toString());
} catch (IOException e) {
// ignore error
LOG.warn("Failed to list directory: {} error: {}", ufsUri, e.toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this message need more detail, since we list in many places and why it is a harmless error?

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 actually changed the error model a little bit. Ultimately, I didn't want the sync to throw an exception. So, this internal method will throw any exception, and the caller, syncMetadata(), will print out an error and return false.

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
}

if (listStatus != null) {
// maps children name to ufs fingerprint
Copy link
Contributor

Choose a reason for hiding this comment

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

to the most up to date ufs fingerprint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

aaudiber
aaudiber suggested changes Jan 4, 2018
View reviewed changes
Copy link
Contributor

@aaudiber aaudiber 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 adding this. Glad to see the master-side changes weren't too extensive. I think it might be cleaner to use a NONE|ALL|ONE_LEVEL enum to represent how to sync/loadMetadata, but you can decide which way you prefer it.

tests/src/test/java/alluxio/master/file/UfsSyncIntegrationTest.java
status = mFileSystem.getStatus(new AlluxioURI(alluxioPath(fileB)));
Assert.assertEquals(ttl, status.getTtl());

// deleted UFS file should not exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

mFileSystem.exists()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

tests/src/test/java/alluxio/master/file/UfsSyncIntegrationTest.java Outdated
writeUfsFile(ufsPath(fileA), 1);
writeUfsFile(ufsPath(fileB), 1);

try {
Copy link
Contributor

Choose a reason for hiding this comment

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

mFileSystem.exists()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
for (UfsStatus childStatus : children) {
boolean skipExisting = inode.getChild(childStatus.getName()) != null;
// Do not skip existing directories if there are more descendant levels to load
if (childStatus.isDirectory() && (options.getLoadDescendantLevels() < 0
Copy link
Contributor

Choose a reason for hiding this comment

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

It could be simpler to have two independent if-continue statements and avoid the need for skipExisting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
continue;
}
TempInodePathForChild tempInodePath =
new TempInodePathForChild(inodePath, status.getName());
new TempInodePathForChild(inodePath, childStatus.getName());
int loadDescendantLevels = options.getLoadDescendantLevels();
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 declare/update this value earlier, we only need to check loadDescendentLevels != 0 on line 2450

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already restructured.

gpang
gpang commented Jan 5, 2018
View reviewed changes
Copy link
Contributor Author

@gpang gpang left a comment

Choose a reason for hiding this comment

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

@calvinjia @aaudiber Thanks! I updated the PR.

I made the descendant type into an Enum.

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
listStatus = ufs.listStatus(ufsUri.toString());
} catch (IOException e) {
// ignore error
LOG.warn("Failed to list directory: {} error: {}", ufsUri, e.toString());
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 actually changed the error model a little bit. Ultimately, I didn't want the sync to throw an exception. So, this internal method will throw any exception, and the caller, syncMetadata(), will print out an error and return false.

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
}

if (listStatus != null) {
// maps children name to ufs fingerprint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

tests/src/test/java/alluxio/master/file/UfsSyncIntegrationTest.java Outdated
writeUfsFile(ufsPath(fileA), 1);
writeUfsFile(ufsPath(fileB), 1);

try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests/src/test/java/alluxio/master/file/UfsSyncIntegrationTest.java
status = mFileSystem.getStatus(new AlluxioURI(alluxioPath(fileB)));
Assert.assertEquals(ttl, status.getTtl());

// deleted UFS file should not exist.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
for (UfsStatus childStatus : children) {
boolean skipExisting = inode.getChild(childStatus.getName()) != null;
// Do not skip existing directories if there are more descendant levels to load
if (childStatus.isDirectory() && (options.getLoadDescendantLevels() < 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

core/server/master/src/main/java/alluxio/master/file/DefaultFileSystemMaster.java Outdated
continue;
}
TempInodePathForChild tempInodePath =
new TempInodePathForChild(inodePath, status.getName());
new TempInodePathForChild(inodePath, childStatus.getName());
int loadDescendantLevels = options.getLoadDescendantLevels();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was already restructured.

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/18216/
Test PASSed.

@calvinjia calvinjia merged commit 9a52ea2 into Alluxio:branch-1.7 Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calvinjia calvinjia calvinjia approved these changes

@aaudiber aaudiber aaudiber approved these changes

Assignees

@apc999 apc999

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gpang @AmplabJenkins @alluxio-bot @calvinjia @aaudiber @apc999