alamb commented on code in PR #4837:
URL: https://github.com/apache/arrow-rs/pull/4837#discussion_r1334762316
##########
object_store/src/client/get.rs:
##########
@@ -49,10 +55,12 @@ impl<T: GetClient> GetClientExt for T {
async fn get_opts(&self, location: &Path, options: GetOptions) ->
Result<GetResult> {
let range = options.range.clone();
let response = self.get_request(location, options, false).await?;
- let meta = header_meta(location, response.headers(),
Default::default())
- .map_err(|e| Error::Generic {
- store: T::STORE,
- source: Box::new(e),
+ let meta =
+ header_meta(location, response.headers(),
T::HEADER_CONFIG).map_err(|e| {
Review Comment:
It took me a while to track down that the default for `HEADER_CONFIG` is the
same as `HeaderConfig::default` and thus this isn't a change for `S3Client` and
other implementations
##########
object_store/src/http/client.rs:
##########
@@ -307,6 +277,58 @@ impl Client {
}
}
+#[async_trait]
+impl GetClient for Client {
+ const STORE: &'static str = "HTTP";
+
+ const HEADER_CONFIG: HeaderConfig = HeaderConfig {
Review Comment:
Perhaps you can add an explanatory comment about why this client overrides
the default HEADER_CONFIG (so as to support http servers that don't implement
WEBDAV?)
##########
object_store/src/client/get.rs:
##########
@@ -28,6 +28,12 @@ use reqwest::Response;
pub trait GetClient: Send + Sync + 'static {
const STORE: &'static str;
+ /// Configure the [`HeaderConfig`] for this client
+ const HEADER_CONFIG: HeaderConfig = HeaderConfig {
+ etag_required: true,
+ last_modified_required: true,
+ };
Review Comment:
I think using `Default` would make the intent clearer here
```suggestion
/// Configure the [`HeaderConfig`] used for requests issued by this
client
const HEADER_CONFIG: HeaderConfig = HeaderConfig::default();
```
--
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]
