andygrove commented on code in PR #2397:
URL:
https://github.com/apache/datafusion-ballista/pull/2397#discussion_r3895839935
##########
ballista/scheduler/src/api/handlers.rs:
##########
@@ -42,40 +42,62 @@ use http::{HeaderMap, StatusCode, header::CONTENT_TYPE};
use std::collections::HashMap;
use std::sync::Arc;
-#[derive(Debug, serde::Serialize)]
-struct SchedulerStateResponse {
- started: u128,
- version: &'static str,
- datafusion_version: &'static str,
- substrait_support: bool,
- keda_support: bool,
- prometheus_support: bool,
- graphviz_support: bool,
- spark_support: bool,
- scheduling_policy: String,
+#[derive(Debug, serde::Serialize, utoipa::ToSchema)]
+pub struct SchedulerStateResponse {
+ pub started: u128,
+ pub version: &'static str,
+ pub datafusion_version: &'static str,
+ pub substrait_support: bool,
+ pub keda_support: bool,
+ pub prometheus_support: bool,
+ pub graphviz_support: bool,
+ pub spark_support: bool,
+ pub scheduling_policy: String,
#[serde(skip_serializing_if = "Option::is_none")]
- advertise_flight_endpoint: Option<String>,
- enable_embedded_flight_proxy: bool,
+ pub advertise_flight_endpoint: Option<String>,
+ pub enable_embedded_flight_proxy: bool,
}
-#[derive(Debug, serde::Serialize)]
-struct SchedulerVersionResponse {
- version: &'static str,
- datafusion_version: &'static str,
+#[derive(Debug, serde::Serialize, utoipa::ToSchema)]
+pub struct SchedulerVersionResponse {
+ pub version: &'static str,
+ pub datafusion_version: &'static str,
}
-#[derive(Debug, serde::Serialize)]
+/// Specification of an executor, indicating its runtime-assigned vcore count.
+#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, utoipa::ToSchema)]
+#[schema(as = ExecutorSpecification)]
+pub struct ExecutorSpecificationSchema {
+ /// Virtual cores assigned to this executor at runtime
+ pub vcores: u32,
+}
+
+/// Operating system level specification of an executor.
+#[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, utoipa::ToSchema)]
+#[schema(as = ExecutorOperatingSystemSpecification)]
+pub struct ExecutorOperatingSystemSpecificationSchema {
Review Comment:
This mirror struct declares three fields, but the real
`ExecutorOperatingSystemSpecification` in
`ballista/core/src/serde/scheduler/mod.rs` has nine. It also carries
`os_ver_long`, `physical_cores`, `num_disks`, `total_disk_space`,
`total_available_disk_space` and `open_files_limit`, and all of them get
serialized.
Since `#[schema(value_type = ...)]` swaps the schema out wholesale, the
published spec will under-describe what `/api/executors` and
`/api/executor/{executor_id}` actually return, right from the first release.
`ExecutorSpecificationSchema` above happens to match today, but it is the same
hand-copied pattern and will drift the same way as soon as someone edits the
core type.
Would it work to derive `ToSchema` on the real types in `ballista-core`
behind an optional feature, the same way you have done it for
`ballista-api-types`? That way there is only one definition to keep right.
If you would rather keep this PR small, filling in the six missing fields
would fix the immediate problem. In that case it might be worth adding a test
that asserts the schema's property set matches the JSON keys of
`ExecutorOperatingSystemSpecification::default()`, so the next drift shows up
as a failing test rather than a quietly wrong spec.
##########
ballista/scheduler/src/api/openapi.rs:
##########
@@ -0,0 +1,256 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! OpenAPI specification for the Ballista scheduler REST API.
+
+use crate::api::SchedulerErrorResponse;
+use crate::api::handlers::{
+ CancelJobResponse, ExecutorMetricResponse,
+ ExecutorOperatingSystemSpecificationSchema, ExecutorResponse,
+ ExecutorSpecificationSchema, JobQueryParams, SchedulerStateResponse,
+ SchedulerVersionResponse,
+};
+use axum::Json;
+use axum::response::IntoResponse;
+use ballista_api_types::dto::{
+ JobResponse, Percentiles, PlanFormat, QueryStageSummary,
QueryStagesResponse,
+ TaskStatus, TaskSummary,
+};
+use utoipa::OpenApi;
+
+/// OpenAPI documentation structure for the Ballista scheduler REST API.
+#[derive(OpenApi)]
+#[openapi(
+ info(
+ title = "Apache DataFusion Ballista Scheduler REST API",
+ version = env!("CARGO_PKG_VERSION"),
+ description = "REST API for Apache DataFusion Ballista Scheduler",
+ license(
+ name = "Apache-2.0",
+ url = "https://www.apache.org/licenses/LICENSE-2.0";
+ )
+ ),
+ paths(
+ crate::api::handlers::get_scheduler_state,
+ crate::api::handlers::get_scheduler_version,
+ crate::api::handlers::get_executors,
+ crate::api::handlers::get_executor_info,
+ crate::api::handlers::get_jobs,
+ crate::api::handlers::get_job,
+ crate::api::handlers::cancel_job,
+ crate::api::handlers::get_job_config,
+ crate::api::handlers::get_query_stages,
+ crate::api::handlers::get_job_dot_graph,
+ crate::api::handlers::get_query_stage_dot_graph,
+ crate::api::handlers::get_scheduler_metrics,
+ crate::api::openapi::get_openapi_spec,
+ ),
+ components(
+ schemas(
+ SchedulerStateResponse,
+ SchedulerVersionResponse,
+ ExecutorResponse,
+ ExecutorMetricResponse,
+ ExecutorSpecificationSchema,
+ ExecutorOperatingSystemSpecificationSchema,
+ CancelJobResponse,
+ JobQueryParams,
+ SchedulerErrorResponse,
+ JobResponse,
+ TaskStatus,
+ TaskSummary,
+ Percentiles,
+ QueryStageSummary,
+ QueryStagesResponse,
+ PlanFormat,
+ )
+ ),
+ tags(
+ (name = "state", description = "Scheduler state and feature
configuration"),
+ (name = "version", description = "Version information"),
+ (name = "executors", description = "Executor management and metrics"),
+ (name = "jobs", description = "Job execution, monitoring, and
cancellation"),
+ (name = "graphs", description = "Execution graph visualization"),
+ (name = "metrics", description = "Prometheus cluster metrics"),
+ (name = "openapi", description = "OpenAPI specification"),
+ )
+)]
+pub struct ApiDoc;
+
+#[cfg(feature = "graphviz-support")]
+#[derive(OpenApi)]
+#[openapi(paths(crate::api::handlers::get_job_svg_graph))]
+struct GraphvizApiDoc;
+
+/// Generate the OpenAPI specification for the scheduler REST API.
+pub fn openapi_spec() -> utoipa::openapi::OpenApi {
Review Comment:
This rebuilds the whole document on every request to `/api/openapi.json`.
The spec is fixed at compile time, so wrapping it in a `std::sync::LazyLock`
would turn the handler into a cheap clone of a cached value.
##########
ballista/api-types/src/dto.rs:
##########
@@ -172,3 +179,20 @@ pub enum PlanFormat {
/// A `BTreeMap` so key order is deterministic: the same job must serialize
/// identically whether it is served live or replayed from a stored log.
pub type JobConfig = BTreeMap<String, String>;
+
+#[cfg(all(test, feature = "utoipa"))]
+mod tests {
+ use super::*;
+ use utoipa::ToSchema;
+
+ #[test]
+ fn test_dto_schemas() {
Review Comment:
This one is mostly asserting that the derive macro does what it says, so I
am not sure it earns its keep. A check that the generated schema's properties
line up with the actual serde output would catch a lot more, things like a
`skip_serializing_if` that the schema does not know about.
Worth knowing too that this only runs when feature unification turns the
`utoipa` feature on from the scheduler side. `cargo test -p ballista-api-types`
on its own will skip it silently.
##########
ballista/scheduler/src/api/openapi.rs:
##########
@@ -0,0 +1,256 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! OpenAPI specification for the Ballista scheduler REST API.
+
+use crate::api::SchedulerErrorResponse;
+use crate::api::handlers::{
+ CancelJobResponse, ExecutorMetricResponse,
+ ExecutorOperatingSystemSpecificationSchema, ExecutorResponse,
+ ExecutorSpecificationSchema, JobQueryParams, SchedulerStateResponse,
+ SchedulerVersionResponse,
+};
+use axum::Json;
+use axum::response::IntoResponse;
+use ballista_api_types::dto::{
+ JobResponse, Percentiles, PlanFormat, QueryStageSummary,
QueryStagesResponse,
+ TaskStatus, TaskSummary,
+};
+use utoipa::OpenApi;
+
+/// OpenAPI documentation structure for the Ballista scheduler REST API.
+#[derive(OpenApi)]
+#[openapi(
+ info(
+ title = "Apache DataFusion Ballista Scheduler REST API",
+ version = env!("CARGO_PKG_VERSION"),
+ description = "REST API for Apache DataFusion Ballista Scheduler",
+ license(
+ name = "Apache-2.0",
+ url = "https://www.apache.org/licenses/LICENSE-2.0";
+ )
+ ),
+ paths(
+ crate::api::handlers::get_scheduler_state,
+ crate::api::handlers::get_scheduler_version,
+ crate::api::handlers::get_executors,
+ crate::api::handlers::get_executor_info,
+ crate::api::handlers::get_jobs,
+ crate::api::handlers::get_job,
+ crate::api::handlers::cancel_job,
+ crate::api::handlers::get_job_config,
+ crate::api::handlers::get_query_stages,
+ crate::api::handlers::get_job_dot_graph,
+ crate::api::handlers::get_query_stage_dot_graph,
+ crate::api::handlers::get_scheduler_metrics,
+ crate::api::openapi::get_openapi_spec,
Review Comment:
`/healthz` and `/readyz` are missing here. They are mounted by
`health_routes` unconditionally, so they are live even when `rest-api` is off,
and they are exactly the two endpoints a platform owner wires into Kubernetes
probes.
Anyone generating a client or a monitoring config from `openapi.json` will
not see them. Happy for this to be a follow-up if you would rather keep Phase 1
focused.
--
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]
