Add named accumulators to track PerfIO S3 backend usage per executor - #14580
Conversation
Signed-off-by: Zach Puller <zpuller@nvidia.com>
Greptile SummaryAdds three named Confidence Score: 5/5Safe to merge — no P0/P1 issues remain; prior review concerns are addressed. All three previously-flagged concerns are resolved: the footer-cache-warm undercount is addressed by the second call site in copyRemoteBlocksData, the unregistered-accumulator throw is guarded by a try/catch, and the reportedBackendAccIds growth was acknowledged as acceptable. Remaining observations are purely P2 style (case-sensitivity of backend name matching, degenerate fully-cached stage where nothing fires). These do not block merge. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[S3 Parquet read task] --> B{fileIO is HadoopFileIO?}
B -- No --> Z[Skip metric]
B -- Yes --> C{filePath scheme starts with 's3'?}
C -- No --> Z
C -- Yes --> D[GpuTaskMetrics.get.recordPerfioS3BackendOnce]
D --> E{PerfIO.s3BackendName}
E -- netty --> F[perfioS3NettyExecutors]
E -- crt --> G[perfioS3CrtExecutors]
E -- other/s3a --> H[perfioS3S3aExecutors]
F --> I{acc.id already in reportedBackendAccIds?}
G --> I
H --> I
I -- Yes --> J[no-op: already counted this stage]
I -- No --> K[acc.add 1]
I -- IllegalArgumentException --> L[catch silently: unregistered accumulator backstop]
subgraph Call sites
CS1[readFooterBuffer footer cache-miss path]
CS2[copyRemoteBlocksData remote data-copy path]
end
CS1 --> D
CS2 --> D
Reviews (3): Last reviewed commit: "catch accumulator not yet registered for..." | Re-trigger Greptile |
| case "crt" => perfioS3CrtExecutors | ||
| case _ => perfioS3S3aExecutors | ||
| } | ||
| if (PerfIO.reportedBackendAccIds.add(acc.id)) { |
There was a problem hiding this comment.
reportedBackendAccIds grows without bound
PerfIO.reportedBackendAccIds is a per-JVM static set that is only ever written to (add), never pruned. Each time a new GpuTaskMetrics instance is created (once per stage per query plan — see GpuTransitionOverrides), up to three new accumulator IDs can be added to this set. Over many queries in a long-running executor JVM the set grows monotonically. Each entry is a boxed Long in a ConcurrentHashMap, so the practical impact is low, but a cleanup hook keyed on stage completion (or resetting at plan-creation time) would prevent unbounded growth.
There was a problem hiding this comment.
At stage-level granularity this shouldn't be an issue
Signed-off-by: Zach Puller <zpuller@nvidia.com>
Signed-off-by: Zach Puller <zpuller@nvidia.com>
|
build |
Description
This change adds three named
LongAccumulatorinstances toGpuTaskMetrics:perfio.s3.netty.executors— executors using the PerfIO Netty backendperfio.s3.crt.executors— executors using the PerfIO CRT backendperfio.s3.s3a.executors— executors falling back to S3AEach accumulator is incremented at most once per executor per stage. The accumulator ID is used as a key in a per-JVM
ConcurrentHashMap-backed set (PerfIO.reportedBackendAccIds) so that even if an executor reads many S3 parquet files in a stage, it contributes exactly 1 to the appropriate counter.The accumulators appear in the stage
Accumulablessection of the Spark event log and History Server UI. On an N-executor cluster where all nodes use PerfIO Netty,perfio.s3.netty.executorswill equal N after any stage that performs S3 parquet reads.Checklists
Documentation
Testing
Performance