andygrove commented on code in PR #2175:
URL:
https://github.com/apache/datafusion-ballista/pull/2175#discussion_r3648778420
##########
ballista/scheduler/src/state/execution_stage.rs:
##########
@@ -218,6 +219,10 @@ pub struct RunningStage {
pub stage_metrics: Option<Vec<MetricsSet>>,
/// [SessionConfig] used for this stage
pub session_config: Arc<SessionConfig>,
+ /// `RuntimeStatsReport`s collected from every successful task in
+ /// this stage attempt. Merged and logged once the stage finalizes;
+ /// dropped when the stage transitions to Successful.
+ pub runtime_stats_reports: Vec<RuntimeStatsReport>,
Review Comment:
Minor, lower stakes than the `as_task_status` one. Adding a public field to
`RunningStage` breaks external struct-literal construction since it is not
`#[non_exhaustive]`. This reads as internal scheduler state built via
`RunningStage::new`, so the real blast radius is probably nil, but flagging it
since it is on the public surface of a `missing_docs` crate. Fine to leave as
is if we are confident nobody constructs this externally, just want it to be a
conscious call.
##########
ballista/executor/src/execution_engine.rs:
##########
@@ -290,6 +302,31 @@ impl QueryStageExecutor for DefaultQueryStageExec {
ShuffleWriterVariant::Sort(writer) =>
utils::collect_plan_metrics(writer),
}
}
+
+ fn collect_runtime_stats_reports(
Review Comment:
This walker now runs once per task at completion on every query, not just
parallel-window plans. I expect it is in the noise since it is a short walk off
the data path, but since it touches the task completion path, could you drop a
quick TPC-H before/after against main just to confirm no regression? A couple
of representative queries is plenty, no need for the full suite:
```sh
cargo run --release --bin tpch -- benchmark ballista \
--host localhost --port 50050 \
--iterations 3 --path <tpch parquet dir> --format parquet --query 1
```
##########
ballista/executor/src/lib.rs:
##########
@@ -103,21 +103,25 @@ pub struct TaskExecutionTimes {
/// This function wraps the outcome of task execution (success or failure)
/// along with timing and metrics information into a status message that
/// can be sent back to the scheduler.
+#[allow(clippy::too_many_arguments)]
pub fn as_task_status(
execution_result: ballista_core::error::Result<Vec<ShuffleWritePartition>>,
executor_id: String,
stage_attempt_num: usize,
key: TaskKey,
operator_metrics: Option<Vec<OperatorMetricsSet>>,
+ runtime_stats: Vec<RuntimeStatsReport>,
Review Comment:
This adds a positional parameter to a public `pub fn` in one of the
`missing_docs` crates, so it is a breaking change for anyone building a custom
executor loop that calls `as_task_status`. Could we make it additive instead?
Given this signature already carries `#[allow(clippy::too_many_arguments)]`,
folding `runtime_stats` into a small completion struct (or the existing
`TaskExecutionTimes`-style param) would avoid the break. If we do decide to
take the break, it needs the `api change` label and an entry under
`docs/source/upgrading/` so library users get a heads up.
Worth noting the trait method you added right next to this is done the
additive way already, with a default impl returning empty, so external
implementors do not break. Same treatment here would be ideal.
--
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]
