andygrove commented on code in PR #6429:
URL: https://github.com/apache/datafusion-comet/pull/6429#discussion_r4196850537
##########
spark/src/test/scala/org/apache/spark/sql/comet/CometTaskMetricsSuite.scala:
##########
@@ -54,6 +54,21 @@ class CometTaskMetricsSuite extends CometTestBase with
AdaptiveSparkPlanHelper {
import testImplicits._
+ test("S3 HTTP metrics are independent scan counters") {
Review Comment:
This test mostly checks how `SQLMetric` behaves, since each
`nativeScanMetrics` call creates fresh `sum` accumulators. It would keep
passing even if the native names never reached the scan node. The existing
`Comet native metrics: scan` test in `CometExecSuite` already lists every
`scan_io_*` name on a real `CometNativeScanExec`. It also asserts that the
`object_store_*` counters stay at zero for a local file. Could you add the
three new names to that list, with `== 0` assertions next to the
`object_store_*` ones, and drop this test? That checks the Spark side against
an actual scan. The zero assertions also pin down the uninstrumented-backend
behavior the docs describe.
##########
docs/source/user-guide/latest/metrics.md:
##########
@@ -205,6 +205,23 @@ metadata and remote totals instead of dividing by zero.
Cancellation can leave l
work outside the final metric snapshot; these counters are not a guarantee of
complete network
traffic accounting after cancellation.
+### S3 HTTP attempts and retries
Review Comment:
Two things here could trip up a reader. The section calls
`object_store_get_calls` "logical" GETs. But further down, the page uses
"logical ranges" for the reader's ranges before coalescing, and
`object_store_get_calls` counts GETs after coalescing. Also, the fact that only
native S3 scans fill these counters is only implied by "An uninstrumented
transport contributes no HTTP coverage". Someone looking at a GCS or Azure scan
will see zeros and could conclude nothing was retried.
Something along these lines might read more easily. The first sentence would
replace the one above the table, and the paragraph would replace the one below
it:
> Native Parquet scans of S3 paths also count the HTTP attempts behind each
GET, without changing the retry policy:
> `scan_io_http_attempts` equals `scan_io_http_observed_gets` plus
`scan_io_http_retries` once the scan finishes. Each scan counts only its own
requests, even when scans share an S3 client. HEAD requests, credential
requests, bucket-region lookups, and redirects or protocol retries handled
inside reqwest are not counted. Only S3 scans fill these counters. GCS, Azure,
HTTP(S), local and HDFS scans always report zero, which does not mean their
requests were never retried. For an S3 scan, `scan_io_http_observed_gets`
normally matches `scan_io_object_store_get_calls`.
##########
spark/src/main/scala/org/apache/spark/sql/comet/CometMetricNode.scala:
##########
@@ -405,6 +405,12 @@ object CometMetricNode {
SQLMetrics.createSizeMetric(sc, "Serialized Parquet footer payload
bytes read"),
"scan_io_object_store_get_calls" ->
SQLMetrics.createMetric(sc, "ObjectStore GET operations after range
coalescing"),
+ "scan_io_http_observed_gets" ->
Review Comment:
The description says the overhead hasn't been measured. `CometReadBenchmark
--scan-metric-overhead` from #5453 measures exactly this per-task accumulator
cost. But it still hard-codes the original nine names and the `9` in its cases,
so after this PR it no longer covers every scan I/O accumulator. #5453 measured
about 41 to 52 µs per task for those nine. That suggests these three add
roughly 15 µs to every native scan task, including local and HDFS scans that
can never report a nonzero value. Could you add the new names to that benchmark
and post the numbers? Also, `http_attempts` always equals `http_observed_gets +
http_retries`. Is the third counter worth its per-task cost, or would two be
enough?
##########
native/core/src/parquet/objectstore/s3.rs:
##########
@@ -189,7 +189,8 @@ impl S3StoreTemplate {
fn build(&self, credentials: S3Credentials) -> Result<AmazonS3,
object_store::Error> {
let builder = AmazonS3Builder::new()
.with_url(self.url.clone())
- .with_allow_http(true);
+ .with_allow_http(true)
+ .with_http_connector(super::http_metrics::ScanHttpConnector);
Review Comment:
Is leaving Azure out deliberate? `objectstore::azure::create_store` builds
its own `MicrosoftAzureBuilder`. object_store's Azure client also forwards
`GetOptions` extensions into its GET requests the same way the S3 client does.
So `.with_http_connector(ScanHttpConnector)` there looks like all it would
need. GCS goes through `parse_url`, so it would need its own builder. If you'd
rather keep this PR to S3, could you open an issue for Azure and GCS coverage?
--
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]
