martin-g commented on code in PR #1506:
URL:
https://github.com/apache/datafusion-ballista/pull/1506#discussion_r2936649932
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -274,13 +278,18 @@ pub async fn get_query_stages<
>(
State(data_server): State<Arc<SchedulerServer<T, U>>>,
Path(job_id): Path<String>,
-) -> Result<impl IntoResponse, StatusCode> {
+) -> Result<impl IntoResponse, SchedulerErrorResponse> {
if let Some(graph) = data_server
.state
.task_manager
.get_job_execution_graph(&job_id)
.await
- .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?
+ .map_err(|e| {
+ SchedulerErrorResponse::with_error(
+ StatusCode::INTERNAL_SERVER_ERROR,
+ e.to_string(),
Review Comment:
Usually the cause is logged and some generic message is sent back to the
client, to prevent showing any sensitive data to users who don't have the
respective permissions.
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -122,15 +123,13 @@ pub async fn get_jobs<
U: AsExecutionPlan + Send + Sync + 'static,
>(
State(data_server): State<Arc<SchedulerServer<T, U>>>,
-) -> Result<impl IntoResponse, StatusCode> {
- // TODO: Display last seen information in UI
+) -> Result<impl IntoResponse, SchedulerErrorResponse> {
let state = &data_server.state;
- let jobs = state
- .task_manager
- .get_jobs()
- .await
- .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+ let jobs =
+ state.task_manager.get_jobs().await.map_err(|_| {
Review Comment:
The error is also not logged by TaskManager::get_jobs().
IMO it should be logger either there or here.
##########
ballista/scheduler/src/scheduler_process.rs:
##########
@@ -130,17 +132,42 @@ pub async fn start_grpc_service<
tonic_builder.add_service(ExternalScalerServer::new(scheduler.clone()));
let tonic = tonic_builder.routes().into_axum_router();
- let tonic = tonic.fallback(|| async { (StatusCode::NOT_FOUND, "404 - Not
Found") });
+
+ // registering default handler for unmatched requests
+ let tonic =
+ tonic.fallback(|| async {
SchedulerErrorResponse::new(StatusCode::NOT_FOUND) });
#[cfg(feature = "rest-api")]
- let axum = get_routes(Arc::new(scheduler));
- #[cfg(feature = "rest-api")]
- let final_route = axum
- .merge(tonic)
- .into_make_service_with_connect_info::<SocketAddr>();
+ let final_route = if config.disable_rest {
+ tonic
+ .route(
+ "/api/{*path}",
+ axum::routing::any(|| async {
+ SchedulerErrorResponse::with_error(
+ StatusCode::NOT_FOUND,
+ "Rest api has been disabled at startup".to_string(),
+ )
+ }),
+ )
+ .into_make_service_with_connect_info::<SocketAddr>()
+ } else {
+ let axum = get_routes(Arc::new(scheduler));
+ axum.merge(tonic)
+ .into_make_service_with_connect_info::<SocketAddr>()
+ };
#[cfg(not(feature = "rest-api"))]
- let final_route =
tonic.into_make_service_with_connect_info::<SocketAddr>();
+ let final_route = tonic
+ .route(
+ "/api/{*path}",
+ axum::routing::any(|| async {
+ SchedulerErrorResponse::with_error(
+ StatusCode::NOT_FOUND,
+ "Rest api has been disabled at compile time".to_string(),
Review Comment:
```suggestion
"REST API has been disabled at compile time".to_string(),
```
##########
ballista/scheduler/src/scheduler_process.rs:
##########
@@ -130,17 +132,42 @@ pub async fn start_grpc_service<
tonic_builder.add_service(ExternalScalerServer::new(scheduler.clone()));
let tonic = tonic_builder.routes().into_axum_router();
- let tonic = tonic.fallback(|| async { (StatusCode::NOT_FOUND, "404 - Not
Found") });
+
+ // registering default handler for unmatched requests
+ let tonic =
+ tonic.fallback(|| async {
SchedulerErrorResponse::new(StatusCode::NOT_FOUND) });
#[cfg(feature = "rest-api")]
- let axum = get_routes(Arc::new(scheduler));
- #[cfg(feature = "rest-api")]
- let final_route = axum
- .merge(tonic)
- .into_make_service_with_connect_info::<SocketAddr>();
+ let final_route = if config.disable_rest {
+ tonic
+ .route(
+ "/api/{*path}",
Review Comment:
I think the routing related code and SchedulerErrorResponse should be in
api/mod.rs.
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -430,16 +439,16 @@ pub async fn get_query_stage_dot_graph<
>(
State(data_server): State<Arc<SchedulerServer<T, U>>>,
Path((job_id, stage_id)): Path<(String, usize)>,
-) -> Result<impl IntoResponse, StatusCode> {
+) -> Result<impl IntoResponse, SchedulerErrorResponse> {
if let Some(graph) = data_server
.state
.task_manager
.get_job_execution_graph(&job_id)
.await
- .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?
+ .map_err(|_|
SchedulerErrorResponse::new(StatusCode::INTERNAL_SERVER_ERROR))?
{
ExecutionGraphDot::generate_for_query_stage(graph.as_ref(), stage_id)
- .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)
+ .map_err(|_|
SchedulerErrorResponse::new(StatusCode::INTERNAL_SERVER_ERROR))
} else {
Ok("Not Found".to_string())
Review Comment:
404 ?!
##########
ballista/scheduler/src/api/mod.rs:
##########
@@ -27,6 +27,7 @@ pub fn get_routes<
) -> Router {
let router = Router::new()
.route("/api/state", get(handlers::get_scheduler_state::<T, U>))
Review Comment:
Is /api/state going to be deprecated in favour of /api/version ?
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -460,7 +469,9 @@ pub async fn get_job_svg_graph<
&mut PrinterContext::default(),
vec![CommandArg::Format(Format::Svg)],
)
- .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+ .map_err(|_| {
Review Comment:
The cause should probably be logger here.
##########
ballista/scheduler/src/api/handlers.rs:
##########
Review Comment:
This should probably return 404
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -216,9 +215,12 @@ pub async fn cancel_job<
.await
.map_err(|err| {
tracing::error!("Error getting job status: {err:?}");
- StatusCode::INTERNAL_SERVER_ERROR
+ SchedulerErrorResponse::with_error(
+ StatusCode::INTERNAL_SERVER_ERROR,
+ format!("Error getting job status: {}", err),
Review Comment:
```suggestion
format!("Error getting job status: {err}"),
```
##########
ballista/scheduler/src/scheduler_process.rs:
##########
@@ -130,17 +132,42 @@ pub async fn start_grpc_service<
tonic_builder.add_service(ExternalScalerServer::new(scheduler.clone()));
let tonic = tonic_builder.routes().into_axum_router();
- let tonic = tonic.fallback(|| async { (StatusCode::NOT_FOUND, "404 - Not
Found") });
+
+ // registering default handler for unmatched requests
+ let tonic =
+ tonic.fallback(|| async {
SchedulerErrorResponse::new(StatusCode::NOT_FOUND) });
#[cfg(feature = "rest-api")]
- let axum = get_routes(Arc::new(scheduler));
- #[cfg(feature = "rest-api")]
- let final_route = axum
- .merge(tonic)
- .into_make_service_with_connect_info::<SocketAddr>();
+ let final_route = if config.disable_rest {
+ tonic
+ .route(
+ "/api/{*path}",
+ axum::routing::any(|| async {
+ SchedulerErrorResponse::with_error(
+ StatusCode::NOT_FOUND,
+ "Rest api has been disabled at startup".to_string(),
Review Comment:
```suggestion
"REST API has been disabled at startup".to_string(),
```
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -409,16 +418,16 @@ pub async fn get_job_dot_graph<
>(
State(data_server): State<Arc<SchedulerServer<T, U>>>,
Path(job_id): Path<String>,
-) -> Result<String, StatusCode> {
+) -> Result<String, SchedulerErrorResponse> {
if let Some(graph) = data_server
.state
.task_manager
.get_job_execution_graph(&job_id)
.await
- .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?
+ .map_err(|_|
SchedulerErrorResponse::new(StatusCode::INTERNAL_SERVER_ERROR))?
{
ExecutionGraphDot::generate(graph.as_ref())
- .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)
+ .map_err(|_|
SchedulerErrorResponse::new(StatusCode::INTERNAL_SERVER_ERROR))
} else {
Ok("Not Found".to_string())
Review Comment:
This is not changed in this PR but it should probably be 404 ?!
##########
ballista/scheduler/src/config.rs:
##########
@@ -190,6 +190,11 @@ pub struct Config {
help = "The interval to check expired or dead executors"
)]
pub expire_dead_executor_interval_seconds: u64,
+
+ #[cfg(feature = "rest-api")]
+ /// Should the rest api be disabled
+ #[arg(long, default_value_t = false, help = "Should the rest api be
disable")]
+ pub disable_rest: bool,
Review Comment:
1) nit: `disable_xyz = false` is a double negation and harder to reason
about than `enable_xyz = true`
2) also consider appending `_api`, i.e. `enable_rest_api = true`. Without
`_api` it may confuse someone that it disables/enables everything else (the
rest).
##########
ballista/scheduler/src/config.rs:
##########
@@ -190,6 +190,11 @@ pub struct Config {
help = "The interval to check expired or dead executors"
)]
pub expire_dead_executor_interval_seconds: u64,
+
+ #[cfg(feature = "rest-api")]
+ /// Should the rest api be disabled
+ #[arg(long, default_value_t = false, help = "Should the rest api be
disable")]
Review Comment:
```suggestion
#[arg(long, default_value_t = false, help = "Should the REST API be
disabled")]
```
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -166,17 +165,17 @@ pub async fn get_job<
>(
State(data_server): State<Arc<SchedulerServer<T, U>>>,
Path(job_id): Path<String>,
-) -> Result<impl IntoResponse, StatusCode> {
+) -> Result<impl IntoResponse, SchedulerErrorResponse> {
let graph = data_server
.state
.task_manager
.get_job_execution_graph(&job_id)
.await
.map_err(|err| {
- tracing::error!("Error occurred while getting the execution graph
for job '{job_id}': {err:?}");
- StatusCode::INTERNAL_SERVER_ERROR
+ tracing::error!("Error occurred while getting the execution graph
for job '{job_id}' reason: {err:?}");
+
SchedulerErrorResponse::with_error(StatusCode::INTERNAL_SERVER_ERROR,
format!("Error occurred while getting the execution graph for job '{job_id}'
reason: {}", err))
Review Comment:
The cause may show too much (e.g. sensitive) information to the end user.
Consider using some generic message in the JSON response.
Only people with access to the logs should be able to see the complete error.
--
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]
