sunchao commented on code in PR #6429:
URL: https://github.com/apache/datafusion-comet/pull/6429#discussion_r4209092562
##########
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:
Addressed in fa501d8fc. Replaced the standalone accumulator test with
assertions in `Comet native metrics: scan` on the real `CometNativeScanExec`.
The remaining two HTTP metric names are required and both must be zero for the
local-file scan, beside the existing object-store zero assertions. I removed
the redundant attempts counter in response to the overhead discussion, so total
attempts are observed GETs plus retries.
The real-scan regression passed with Spark 4.0.4 and JDK 17 using the
freshly built native library.
##########
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:
Addressed in fa501d8fc. Reworded this around GETs after range coalescing and
made the backend scope explicit. The section now says GCS, Azure, HTTP(S),
local and HDFS report zero without proving there were no retries, including S3
paths routed through `fs.comet.libhdfs.schemes`. It also lists the excluded
request types and location-scoped credential re-routing. Total attempts are
documented as observed GETs plus retries after removing the redundant counter.
##########
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:
Addressed in fa501d8fc. Agreed about the third counter: I removed
`scan_io_http_attempts` from the native and Spark metrics. Total attempts are
`scan_io_http_observed_gets + scan_io_http_retries`, so the two remaining
counters preserve coverage and retry information.
The benchmark now includes all eleven current metrics, with the original
nine and a hypothetical twelfth attempts counter for comparison. Labels derive
from the counts, and `--reverse-cases` reverses all comparisons.
Accumulator benchmark: Spark 4.0.4, OpenJDK 17.0.20.1, AMD EPYC-Milan,
`local[1]`, 4 GiB heap. Forward and reverse case orders ran in separate JVMs
using the current reactor classes. For each 10,000-task job case, the table
subtracts the zero-accumulator baseline from the best of three measured
iterations. Baseline task times were 634.9 µs forward and 649.6 µs reverse.
| Scan I/O accumulators | Forward extra µs/task | Reverse extra µs/task |
| --- | ---: | ---: |
| 9 (original) | 44.1 | 39.8 |
| 11 (this revision) | 47.7 | 46.8 |
| 12 (including redundant attempts) | 49.6 | 72.6 |
The two new counters added 3.6 µs/task versus the original nine in the
forward run and 7.0 µs/task in reverse. The redundant twelfth counter's
marginal result varied substantially with case order, so these runs do not
establish a fixed cost per counter. This benchmark measures JVM accumulator
creation, copying, updates, merging, serialization, and scheduling. It does not
measure native HTTP instrumentation, JNI traversal, UI rendering, storage I/O,
or query performance. Native instrumentation overhead remains unmeasured.
For completeness, driver creation was 4.62/6.10/6.74 µs per operator for
9/11/12 metrics in forward order, and 5.90/5.91/6.10 µs in reverse.
Copy/update/merge was 15.0/19.1/21.4 ns per task forward and 15.3/19.1/21.2 ns
reverse. These tight-loop numbers should not be substituted for the Spark-job
measurements above.
I used Spark 4.0 because the configured Maven mirror failed DNS resolution
for the Spark 4.1 dependency `jackson-bom:2.21.2`. Local 4.1 results are not
claimed.
##########
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:
Addressed in fa501d8fc. Yes, I kept this PR scoped to S3 and opened #6748
for Azure and GCS. The follow-up covers adding the connector to the existing
Azure builder and an explicit GCS construction path that preserves the current
`parse_url` behavior, plus retry/resume and shared-client attribution tests.
Both builders in object_store 0.13.2 support `with_http_connector`, and both
GET paths forward the request extensions.
##########
native/core/src/parquet/objectstore/http_metrics.rs:
##########
@@ -0,0 +1,327 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Scan attribution at object_store's HTTP connector boundary.
+//!
+//! The connector delegates client construction, requests, responses, and
errors unchanged.
+//! Counts include object_store retries of HTTP errors and interrupted
response bodies, but
+//! exclude redirects and protocol retries performed internally by reqwest,
credential requests,
+//! and bucket-region discovery. They are not a count of every request
transmitted on the wire.
Review Comment:
Addressed in fa501d8fc. Added the location-scoped re-route to the module
comment and user guide, including the warning that retries can reflect stale
location credentials rather than throttling.
The new loopback regression goes through the real
`LocationScopedObjectStore::get_opts`: the old route returns 403, refreshing
the location list selects a different instrumented S3 store, and that route
succeeds. It asserts one observed GET and one retry, hence two total attempts.
The separate attempts counter has been removed in response to the overhead
discussion. All five HTTP tests passed as part of 234 passing native Parquet
tests.
--
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]
