tustvold commented on code in PR #2374:
URL: https://github.com/apache/arrow-rs/pull/2374#discussion_r942702801
##########
object_store/src/azure.rs:
##########
@@ -277,79 +277,92 @@ impl ObjectStore for MicrosoftAzure {
}
async fn get(&self, location: &Path) -> Result<GetResult> {
- let blob = self
+ let loc = location.clone();
+ let stream = self
.container_client
- .as_blob_client(location.as_ref())
+ .blob_client(location.as_ref())
.get()
- .execute()
- .await
- .map_err(|err| {
- if check_err_not_found(&err) {
- return Error::NotFound {
- source: err,
- path: location.to_string(),
- };
- };
- Error::UnableToGetData {
- source: err,
- container: self.container_name.clone(),
- path: location.to_string(),
- }
- })?;
+ .into_stream()
+ .and_then(|chunk| chunk.data.collect())
+ .map_err(move |err| match err.kind() {
+ AzureErrorKind::HttpResponse {
+ status: StatusCode::NotFound,
+ ..
+ } => crate::Error::NotFound {
Review Comment:
:heart:
##########
object_store/src/azure.rs:
##########
@@ -673,25 +700,41 @@ impl MicrosoftAzureBuilder {
(true, storage_client)
} else {
- (
- false,
- StorageAccountClient::new_access_key(
- Arc::clone(&http_client),
+ let client = if bearer_token.is_some() {
+ Ok(StorageClient::new_bearer_token(
&account,
- &access_key,
- ),
- )
+ bearer_token.unwrap(),
+ ))
+ } else if access_key.is_some() {
+ Ok(StorageClient::new_access_key(&account,
access_key.unwrap()))
+ } else if client_id.is_some()
Review Comment:
You could use something like
```
if let (Some(client_id), Some(client_secret), Some(tenant_id)) = (tenant_id,
client_id, client_secret) {
```
And avoid the unwraps
##########
object_store/src/azure.rs:
##########
@@ -673,25 +700,41 @@ impl MicrosoftAzureBuilder {
(true, storage_client)
} else {
- (
- false,
- StorageAccountClient::new_access_key(
- Arc::clone(&http_client),
+ let client = if bearer_token.is_some() {
Review Comment:
`if let Some(bearer_token) = bearer_token` might be a nicer way to write
this as you can avoid the unwrap below
##########
object_store/src/azure.rs:
##########
@@ -277,79 +277,92 @@ impl ObjectStore for MicrosoftAzure {
}
async fn get(&self, location: &Path) -> Result<GetResult> {
- let blob = self
+ let loc = location.clone();
+ let stream = self
.container_client
- .as_blob_client(location.as_ref())
+ .blob_client(location.as_ref())
.get()
- .execute()
- .await
- .map_err(|err| {
- if check_err_not_found(&err) {
- return Error::NotFound {
- source: err,
- path: location.to_string(),
- };
- };
- Error::UnableToGetData {
- source: err,
- container: self.container_name.clone(),
- path: location.to_string(),
- }
- })?;
+ .into_stream()
+ .and_then(|chunk| chunk.data.collect())
+ .map_err(move |err| match err.kind() {
+ AzureErrorKind::HttpResponse {
+ status: StatusCode::NotFound,
+ ..
+ } => crate::Error::NotFound {
+ source: Box::new(err),
+ path: loc.to_string(),
+ },
+ _ => crate::Error::Generic {
+ source: Box::new(err),
+ store: "MicrosoftAzure",
Review Comment:
Yeah, this will be unavoidable - `GetResult::Stream` wants a `'static`
lifetime on the stream, this prevents bundling a reference to `location` into
the stream, as this would not be `'static`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
