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.

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

Cut over to Path API for file deletion #8366

Merged
merged 1 commit into from Nov 6, 2014

Conversation

s1monw
Copy link
Contributor

@s1monw s1monw commented Nov 6, 2014

Today we use the File API for file deletion as well as recursive
directory deletions. This API returns a boolean if operations
are successful while hiding the actual reason why they failed.
The Path API throws and actual exception that might provide better
insights and debug information.

return true;
} catch (Exception e) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems weird to me to catch any Exception given that the method signature throws IOException?

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 agree will throw the exception

@s1monw
Copy link
Contributor Author

s1monw commented Nov 6, 2014

@jpountz @rmuir pushed an update

try {
IOUtils.rm(FileSystemUtils.toPaths(nodeDatas));
} catch (Exception ex) {
logger.debug("failed to remove node data locations", ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

indentation

@jpountz
Copy link
Contributor

jpountz commented Nov 6, 2014

LGTM

@s1monw
Copy link
Contributor Author

s1monw commented Nov 6, 2014

pushed another review round

Today we use the File API for file deletion as well as recursive
directory deletions. This API returns a boolean if operations
are successful while hiding the actual reason why they failed.
The Path API throws and actual exception that might provide better
insights and debug information.

Closes elastic#8366
@s1monw s1monw merged commit 95171e2 into elastic:master Nov 6, 2014
@s1monw s1monw deleted the ban_file_delete branch November 6, 2014 16:33
@s1monw
Copy link
Contributor Author

s1monw commented Nov 6, 2014

I marked it as breaking since I changed some interfaces internally

tlrx added a commit to tlrx/elasticsearch-cloud-aws that referenced this pull request Nov 7, 2014
tlrx added a commit to tlrx/elasticsearch-cloud-gce that referenced this pull request Nov 7, 2014
dadoonet added a commit to elastic/elasticsearch-cloud-azure that referenced this pull request Nov 12, 2014
tlrx added a commit to tlrx/elasticsearch-cloud-gce that referenced this pull request Nov 14, 2014
@clintongormley clintongormley changed the title [CORE] Cut over to Path API for file deletion Cut over to Path API for file deletion Jun 6, 2015
@clintongormley clintongormley added the :Core/Infra/Core Core issues without another label label Jun 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants