-
Notifications
You must be signed in to change notification settings - Fork 283
[datalake] Migrate file system operations to pipeline architecture #597
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
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.
Looks good! I had a few nit picks that I think would be worth fixing before merging, but I'll still mark this PR as approved.
@@ -81,6 +81,12 @@ impl From<azure_core::HttpError> for Error { | |||
} | |||
} | |||
|
|||
impl From<azure_core::StreamError> for Error { | |||
fn from(error: azure_core::StreamError) -> Self { | |||
Self::CoreError(azure_core::Error::Stream(error)) |
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.
I'll make a pass over the storage errors after core & identity ship, but if the goal here is to put all azure_core::Error
errors into Self::CoreError
, then it can be done with a generic similar to:
azure-sdk-for-rust/sdk/data_cosmos/src/errors.rs
Lines 60 to 64 in 3e92851
impl<T: Into<azure_core::ParsingError>> From<T> for ParsingError { | |
fn from(error: T) -> Self { | |
Self::Core(error.into()) | |
} | |
} |
but with Into<azure_core::Error>
.
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.
I think this needed to be the variant, since other core variants are handled above in more specific manner. this then leads to duplicate implementations.
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.
my bad - this is not even a variant. However i tried your approach and there is conflicting implementations then, and I did not want to change around the error too much in this PR.
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.
Sounds good.
Sorry, @roeap just getting to your question about using |
In this PR we migrate the file system operations which were not using the pipeline architecture to ... the pipeline architecture. Generally the
into_future
andinto_stream
pattern is employed.A few things to note:
Cow<'static, str>
, I don't feel that I can give a qualified statement on what is better, just a gut feeling ...Cow
.StorageCredentials
enum in the storage crate, should they be used instead of the newStorageSharedKeyCredential
(@thovoll)into_stream
onListFileSystems
I moved theBox::pin()
into the function call, so that the caller would not have to worry about that, but I am not sure if there are any implications I missed. Just running the code works :). (@rylev)DataLakeClient
constriuctor, I made it fallible. THis is probably not desired, but my thinking was to accept this for now, and address this in a follow up PR when the constructirs are updated based on How do we set Pipelines options to a Client? #591As a next step I'd like to move the file operations and with that enable tests using the mock framework. I already have these changes ready to go from the large PR.
Once that is done, I think the initialization for the client needs to be revised based on #591 - I'd be happy to give that a shot as well, since for me as a user of the crates this is a very important change :).