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

Add 'get_status' to datalake file client #722

Merged
merged 29 commits into from
Apr 29, 2022
Merged

Add 'get_status' to datalake file client #722

merged 29 commits into from
Apr 29, 2022

Conversation

rickrain
Copy link

This implements get_status for the datalake file client, as mentioned in #496.

@rickrain
Copy link
Author

Ping... Any thoughts yet on this PR? It's really pretty lightweight/simple. Most of the files in this PR are due to the new test (didn't exist before) and all the transaction files needed to pass CI checks.

common_storage_response_headers: headers.try_into()?,
etag: etag_from_headers(headers)?,
last_modified: last_modified_from_headers(headers)?,
properties: headers.try_into().ok(),
Copy link
Member

@cataggar cataggar Apr 27, 2022

Choose a reason for hiding this comment

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

Thanks for the details on why it is Option<Properties> now, but why use ok() here instead of ??

Copy link
Author

Choose a reason for hiding this comment

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

Ahh, that's because I need to convert Result<T, E> to Option<T> since properties is now an Option. However, I just realized a problem with this whereby the E get's discarded silently in this implementation. A better implementation would be to only discard the error if it is HeaderNotFound since this is now an expected condition, and propagate all other errors that may come out of try_into(). So, basically this:

properties: match headers.try_into() {
    Ok(p) => Some(p),
    // 'x-ms-properties' will not be returned in some cases, such as when the request 'action' is set to 'getStatus'
    Err(crate::Error::HeaderNotFound(_)) => None,
    // Propogate all other errors
    Err(e) => return Err(e),
},

I just tested this locally and it works as expected. If you're OK with this implementation, I'll push it into my PR.

Copy link
Author

Choose a reason for hiding this comment

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

@cataggar, just heads up that I went ahead and applied the change above. Let me know if this resolves your concerns.

Copy link
Member

@cataggar cataggar Apr 29, 2022

Choose a reason for hiding this comment

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

Sorry @rickrain, I'm lacking time to dig into this. An error case shouldn't be a normal workflow. If the header is optional for an operation and it is not found, it should return None. It should not return an error and then map that to None.

Copy link
Author

Choose a reason for hiding this comment

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

Ok. So, you would rather me write the same logic that try_into() already does so I can check if the header is there first, rather than letting try_into() just return the error and respond accordingly. This seems to be very opinionated, but ok. In other languages, where exceptions are expensive, I would agree with this opinion. But, I'm not sure I'm in agreement here given how errors are handled in Rust.

Copy link
Member

@cataggar cataggar Apr 29, 2022

Choose a reason for hiding this comment

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

You can use get_option_str_from_headers rickrain#3 .

Copy link
Author

Choose a reason for hiding this comment

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

I had no idea that function existed. Thank you for steering me in the right direction! Had a different implementation I was about to commit but like yours better given it's using an existing function from the SDK.

use get_option_str_from_headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants