martin-g commented on code in PR #1788:
URL:
https://github.com/apache/datafusion-ballista/pull/1788#discussion_r3320217199
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -523,11 +543,19 @@ pub async fn get_query_stages<
};
match stage {
ExecutionStage::Running(running_stage) => {
- summary.stage_plan = if render_tree {
-
Some(displayable(running_stage.plan.as_ref()).tree_render().to_string())
+ let stage_metrics = if let Some(ref m) =
running_stage.stage_metrics {
+ m.to_vec()
} else {
-
Some(displayable(running_stage.plan.as_ref()).indent(false).to_string())
+ Vec::new()
};
Review Comment:
No need to clone the metrics.
```suggestion
let empty_metrics = Vec::new();
let stage_metrics = running_stage
.stage_metrics
.as_ref()
.unwrap_or(&empty_metrics);
```
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -523,11 +543,19 @@ pub async fn get_query_stages<
};
match stage {
ExecutionStage::Running(running_stage) => {
- summary.stage_plan = if render_tree {
-
Some(displayable(running_stage.plan.as_ref()).tree_render().to_string())
+ let stage_metrics = if let Some(ref m) =
running_stage.stage_metrics {
+ m.to_vec()
} else {
-
Some(displayable(running_stage.plan.as_ref()).indent(false).to_string())
+ Vec::new()
};
+ summary.stage_plan = Some(
+ DisplayableBallistaExecutionPlan::new(
+ running_stage.plan.as_ref(),
+ &stage_metrics,
Review Comment:
```suggestion
stage_metrics,
```
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -31,6 +31,7 @@ use ballista_core::serde::scheduler::{
};
use ballista_core::utils::get_current_time;
use datafusion::DATAFUSION_VERSION;
+use crate::display::DisplayableBallistaExecutionPlan;
Review Comment:
Please move it to the group `use crate::...`
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -370,7 +371,26 @@ pub async fn get_job<
SchedulerErrorResponse::with_error(StatusCode::INTERNAL_SERVER_ERROR,
format!("Error occurred while getting the execution graph for job '{job_id}'"))
})?
.ok_or_else(|| SchedulerErrorResponse::new(StatusCode::NOT_FOUND))?;
- let stage_plan = format!("{:?}", graph);
+ let stage_plan = {
+ let plans: Vec<String> = graph
+ .as_ref()
+ .stages()
+ .iter()
+ .filter_map(|(id, stage)| {
+ match stage {
+ ExecutionStage::Successful(completed) => {
+ let displayable =
DisplayableBallistaExecutionPlan::new(
+ completed.plan.as_ref(),
+ &completed.stage_metrics,
+ );
+ Some(format!("Stage {}: {}", id,
displayable.concise()))
+ }
+ _ => None,
Review Comment:
Why the other types are ignored ?
Failed and Running also have both the plan and the metrics.
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -490,7 +510,7 @@ pub async fn get_query_stages<
Path(job_id): Path<String>,
query: Query<JobQueryParams>,
) -> Result<impl IntoResponse, SchedulerErrorResponse> {
- let render_tree = query.render_tree.unwrap_or(false);
+ let _render_tree = query.render_tree.unwrap_or(false);
Review Comment:
Why is this functionality removed ?
It was added recently and it is still needed. Actually it is extended at
https://github.com/apache/datafusion-ballista/pull/1778/changes
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -523,11 +543,19 @@ pub async fn get_query_stages<
};
match stage {
ExecutionStage::Running(running_stage) => {
- summary.stage_plan = if render_tree {
-
Some(displayable(running_stage.plan.as_ref()).tree_render().to_string())
+ let stage_metrics = if let Some(ref m) =
running_stage.stage_metrics {
+ m.to_vec()
} else {
-
Some(displayable(running_stage.plan.as_ref()).indent(false).to_string())
+ Vec::new()
};
Review Comment:
Actually there is no need to get a reference to the actual metrics at all.
`concise()` don't need them at all.
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -523,11 +543,19 @@ pub async fn get_query_stages<
};
match stage {
ExecutionStage::Running(running_stage) => {
- summary.stage_plan = if render_tree {
-
Some(displayable(running_stage.plan.as_ref()).tree_render().to_string())
+ let stage_metrics = if let Some(ref m) =
running_stage.stage_metrics {
Review Comment:
```suggestion
let stage_metrics = if let Some(ref m) =
running_stage.stage_metrics {
```
--
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]
