martin-g commented on code in PR #1498:
URL:
https://github.com/apache/datafusion-ballista/pull/1498#discussion_r2916250119
##########
ballista/scheduler/src/state/mod.rs:
##########
@@ -443,11 +443,16 @@ impl<T: 'static + AsLogicalPlan, U: 'static +
AsExecutionPlan> SchedulerState<T,
None
};
+ let logical_plan_str = plan.display_indent().to_string();
+
let plan = session_ctx.state().create_physical_plan(plan).await?;
debug!(
"Physical plan: {}",
DisplayableExecutionPlan::new(plan.as_ref()).indent(false)
);
+ let physical_plan_str = DisplayableExecutionPlan::new(plan.as_ref())
+ .indent(false)
+ .to_string();
let plan = plan.transform_down(&|node: Arc<dyn ExecutionPlan>| {
Review Comment:
`transform_down()` may replace nodes.
Is `physical_plan_str` intentionally initialized before the transformation ?
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -56,6 +56,9 @@ pub struct JobResponse {
pub num_stages: usize,
pub completed_stages: usize,
pub percent_complete: u8,
+ pub logical_plan: Option<String>,
+ pub physical_plan: Option<String>,
+ pub stage_plan: Option<String>,
Review Comment:
Maybe it would be a good idea to use a different struct for Job detail
response.
get_jobs() does not need to populate the plans.
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -56,6 +56,9 @@ pub struct JobResponse {
pub num_stages: usize,
pub completed_stages: usize,
pub percent_complete: u8,
+ pub logical_plan: Option<String>,
+ pub physical_plan: Option<String>,
+ pub stage_plan: Option<String>,
Review Comment:
```suggestion
#[serde(skip_serializing_if = "Option::is_none")]
pub logical_plan: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub physical_plan: Option<String>,
#[serde(skip_serializing_if = "Option::is_none")]
pub stage_plan: Option<String>,
```
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -172,13 +175,85 @@ pub async fn get_jobs<
num_stages: job.num_stages,
completed_stages: job.completed_stages,
percent_complete,
+ logical_plan: None,
+ physical_plan: None,
+ stage_plan: None,
}
})
.collect();
Ok(Json(jobs))
}
+pub async fn get_job<
+ T: AsLogicalPlan + Clone + Send + Sync + 'static,
+ U: AsExecutionPlan + Send + Sync + 'static,
+>(
+ State(data_server): State<Arc<SchedulerServer<T, U>>>,
+ Path(job_id): Path<String>,
+) -> Result<impl IntoResponse, StatusCode> {
+ let graph = data_server
+ .state
+ .task_manager
+ .get_job_execution_graph(&job_id)
+ .await
+ .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?
Review Comment:
```suggestion
.map_err(|err| {
tracing::error!("Error occurred while getting the execution
graph for job '{job_is}': {err:?}");
StatusCode::INTERNAL_SERVER_ERROR
})?
```
##########
ballista/scheduler/src/api/mod.rs:
##########
@@ -31,6 +31,7 @@ pub fn get_routes<
.route("/api/executors", get(handlers::get_executors::<T, U>))
.route("/api/jobs", get(handlers::get_jobs::<T, U>))
.route("/api/job/{job_id}", patch(handlers::cancel_job::<T, U>))
+ .route("/api/job/{job_id}", get(handlers::get_job::<T, U>))
Review Comment:
Chaining the handlers is more idiomatic Axum:
```suggestion
.route("/api/job/{job_id}", get(handlers::get_job::<T,
U>).patch(handlers::cancel_job::<T, U>))
```
##########
ballista/scheduler/src/state/mod.rs:
##########
@@ -443,11 +443,16 @@ impl<T: 'static + AsLogicalPlan, U: 'static +
AsExecutionPlan> SchedulerState<T,
None
};
+ let logical_plan_str = plan.display_indent().to_string();
+
let plan = session_ctx.state().create_physical_plan(plan).await?;
debug!(
"Physical plan: {}",
DisplayableExecutionPlan::new(plan.as_ref()).indent(false)
);
+ let physical_plan_str = DisplayableExecutionPlan::new(plan.as_ref())
+ .indent(false)
Review Comment:
One concern here: The plan could be for an SQL query that contains sensitive
data. Since Ballista REST API is not authenticated/authorised this may lead to
leaks.
##########
ballista/scheduler/Cargo.toml:
##########
@@ -34,7 +34,7 @@ required-features = ["build-binary"]
[features]
build-binary = ["clap", "tracing-subscriber", "tracing-appender", "tracing",
"ballista-core/build-binary"]
-default = ["build-binary", "substrait"]
+default = ["build-binary", "rest-api"]
Review Comment:
Is this intentional ?
--
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]
