andygrove commented on PR #5453:
URL:
https://github.com/apache/datafusion-comet/pull/5453#issuecomment-5607723408
The rebase brought in more than a replay, and one piece of it interacts with
the feature this PR adds.
Against `130ee02bb`, the head I approved, the metrics work itself is
unchanged. What moved is inherited: `Arc<FileMetadataCache>` instead of
`Arc<dyn FileMetadataCache>`, and `prepare_object_store_with_configs` now opens
with `main`'s
```rust
let url = normalize_object_store_url(url.as_str(), object_store_configs)?;
let is_hdfs_scheme = is_hdfs_scheme(&url, object_store_configs);
```
That ordering, from #5314, classifies libhdfs routing against the
*rewritten* scheme, so an `s3a://` URL with `fs.comet.libhdfs.schemes=s3` and
no `s3a` entry normalizes to `s3://` and then matches the list. I confirmed it
on `424c31aa7` rather than reading it off the diff:
```
PROBE typed scheme = s3a
PROBE typed is_hdfs = false
PROBE normalized scheme = s3
PROBE final is_hdfs = true
```
I filed that as #5816, and it turned up while chasing the failure on #5503,
whose `isolates_backends_even_when_s3_alias_and_configs_match` covers exactly
that pair.
The reason it matters here rather than only there is that
`object_store_backend` takes `is_hdfs` as its first, precedence-taking input:
```rust
if is_hdfs {
return Ok(ObjectStoreBackend::Other);
}
```
So under that configuration an `s3a://` scan is classified `Other` while it
is in fact served by the native S3 store, and every counter gated on `Remote`
reads zero. That is the read-amplification view this PR exists to provide,
going silently absent for a real S3 configuration, and it is exactly the
failure mode your own comment warns against: "Callers must use the returned
backend classification rather than infer it from an original scheme alias." The
classification is right to be centralised; its `is_hdfs` input is what is wrong.
This does not change my view of the metrics work, and I am not asking you to
widen this PR's scope. But since #5453 depends on #5503 by your own note, and
#5503's test is the natural regression guard for #5816, could the ordering fix
land in #5503 and this rebase onto it? Then `ObjectStoreBackend` here is fed a
correct decision on arrival rather than inheriting a wrong one. If you would
rather keep them independent, a `Remote` versus `Other` assertion for `s3a`
under `fs.comet.libhdfs.schemes=s3` in this PR's tests would at least fail
loudly instead of reporting zeros.
My approval stands for everything else. `metrics.md`, the nine counters and
the double-counting note are all as I reviewed them.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
