Re: [PR] feat: expose per-scan S3 HTTP attempts and retries [datafusion-comet]

["andygrove (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org