-
Notifications
You must be signed in to change notification settings - Fork 182
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
Improve error logs for unsupported operations: File Overwrite, Random Write, Directory Shadowing, Unlink (without mount option) #699
Conversation
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.
@sauraank thanks for improving the logs! I have put a few comments.
mountpoint-s3/src/inode.rs
Outdated
WriteStatus::Remote => Err(InodeError::InodeNotWritable(inode.err())), | ||
WriteStatus::Remote => Err(InodeError::InodeNotWritable( | ||
inode.err(), | ||
"Writing to existing file is NOT supported by Mountpoint.".to_owned(), |
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.
Rather than adding a message here, I'd prefer having a specific InodeError
case for each situation.
Signed-off-by: Ankit Saurabh <[email protected]>
ded1ff4
to
f0998eb
Compare
Signed-off-by: Ankit Saurabh <[email protected]>
Signed-off-by: Ankit Saurabh <[email protected]>
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.
LGTM
Description of change
Added clear message for the operations that are unsupported but did not had easy to understand logs. Added links to semantics for deletion and allowed filenames to be visible using Mountpoint. But, with links there is a risk that they can get broken once the content is moved somewhere else.
I will add another PR for troubleshooting page and down level the error noise to
debug!
in other Pull Requests.With this new change, following are error logs that we receive:
For directory shadowing of file with same name,
ls
give following output -For file deletion without using --allow-delete flag, give following in logs -
Out of Write gives -
UPDATE: Overwriting a file is now supported, so the error message is the one set in PR #487 -
Also changed the logging in case of finish writing to update status of Inode, but the Inode WriteStatus is in Invalid state. Earlier it was
InodeNotWritable
which is same error for File Overwrite when its flag is not enabled. Also, it did not completely explained that WriteStatus of the Inode is not valid for writing.Relevant issues:
#533
Does this change impact existing behavior?
No. It only improves the error logging as mentioned above.
No, there is no breaking change.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).