sunchao commented on code in PR #7420: URL: https://github.com/apache/datafusion-comet/pull/7420#discussion_r4447108837
########## native/native-executors/src/bin/comet-executor.rs: ########## @@ -26,6 +26,11 @@ //! - `COMET_EXECUTOR_GRPC_PORT` (default `50052`) //! - `COMET_SCHEDULER_HOST` (default `localhost`) and `COMET_SCHEDULER_PORT` (default `50050`) //! - `COMET_EXECUTOR_TASKS`: concurrent tasks (default: the number of CPUs) +//! - `COMET_GRPC_MAX_MESSAGE_SIZE`: the largest gRPC message, in bytes, the executor accepts or +//! sends, such as a task with many partitions of a scan (default: Ballista's 16 MiB) +//! - `COMET_EXECUTOR_MEMORY_POOL_SIZE`: the bytes of memory the executor's tasks share (default: +//! Ballista's, 70% of the container's memory limit). Memory outside the pool, such as batches Review Comment: Could this describe a budget split into per-task pools and document that `0` selects an unbounded pool? Ballista divides the configured bytes by the executor’s vcores, then gives each task a separate pool proportional to the vcores it claims. A one-vcore task on an eight-vcore executor gets one eighth of the budget even if it’s the only task running. When unset, Ballista uses 70% of the smaller detected cgroup limit or host memory, with an unbounded fallback if neither is detected. [Budget resolution](https://github.com/apache/datafusion-ballista/blob/55.0.0/ballista/executor/src/executor_process.rs#L100-L163) · [Per-task pool policy](https://github.com/apache/datafusion-ballista/blob/55.0.0/ballista/executor/src/executor_process.rs#L216-L245) ########## native/native-executors/Cargo.toml: ########## @@ -37,7 +37,9 @@ publish = false arrow-flight = "59.2.0" ballista-core = "55.0.0" ballista-executor = { version = "55.0.0", default-features = false, features = ["arrow-ipc-optimizations"] } -ballista-scheduler = { version = "55.0.0", default-features = false } +# `rest-api` compiles the scheduler's event-log writer (COMET_SCHEDULER_EVENT_LOG_DIR) and its REST +# API. In Ballista 55.0.0 it logs through `tracing` without enabling it, so that is enabled here. +ballista-scheduler = { version = "55.0.0", default-features = false, features = ["rest-api", "tracing"] } Review Comment: Could we keep the Ballista REST routes disabled in Comet’s `SchedulerConfig`? Enabling `rest-api` makes `start_grpc_service` mount `/api/jobs`, job plans, and `PATCH /api/job/{id}` on the executor-facing scheduler port. A network peer can enumerate and cancel jobs over HTTP without authentication, even with `COMET_SCHEDULER_EVENT_LOG_DIR` unset. Setting `disable_rest_api: true` keeps those routes off while `event_log_dir` still initializes the writer. Could we use `Cluster::start_with` to run a small query with logging enabled, wait for a finalized `.eventlog`, and assert that `GET /api/jobs` returns 404 on the scheduler port? [Route setup](https://github.com/apache/datafusion-ballista/blob/55.0.0/ballista/scheduler/src/scheduler_process.rs#L218-L237) · [REST endpoints](https://github.com/apache/datafusion-ballista/blob/55.0.0/ballista/scheduler/src/api/routes.rs#L32-L73) · [Writer setup](https://github.com/apache/datafusion-ballista/blob/55.0.0/ballista/scheduler/src/schedule r_server/mod.rs#L211-L224) ########## native/native-executors/src/bin/comet-scheduler.rs: ########## @@ -20,10 +20,17 @@ //! //! Configured through environment variables, with Ballista's defaults otherwise: //! - `COMET_SCHEDULER_BIND_HOST` (default `127.0.0.1`) +//! - `COMET_SCHEDULER_EXTERNAL_HOST`: the host executors reach this scheduler at, which they +//! report task status to (default `localhost`) //! - `COMET_SCHEDULER_PORT`: Ballista's gRPC port, for executors (default `50050`) //! - `COMET_SCHEDULER_FLIGHT_PORT`: the Flight port, for drivers (default `50060`) //! - `COMET_SCHEDULER_RESULT_RETENTION_SECONDS`: how long a query's results stay readable after //! the query ends, whether or not a driver has read them (default `300`). `0` keeps them. +//! - `COMET_GRPC_MAX_MESSAGE_SIZE`: the largest gRPC message, in bytes, the scheduler sends or +//! accepts, such as a task with many partitions of a scan (default: Ballista's 16 MiB) Review Comment: Could the docs scope this to the Ballista RPC endpoints and tell users to set it on both scheduler and executors for large task messages? Comet’s separate Spark-facing Flight listener still sets both limits to `usize::MAX` below. The executor Flight server picks up this setting, while remote shuffle Flight clients keep the separate 16 MiB session default. Raising the variable on every process therefore raises the task RPC limit but can still leave a large shuffle response above the receiving client’s limit. [Shuffle client configuration](https://github.com/apache/datafusion-ballista/blob/55.0.0/ballista/core/src/execution_plans/shuffle_reader.rs#L1034-L1055) ########## native/native-executors/src/bin/comet-scheduler.rs: ########## @@ -20,10 +20,17 @@ //! //! Configured through environment variables, with Ballista's defaults otherwise: //! - `COMET_SCHEDULER_BIND_HOST` (default `127.0.0.1`) +//! - `COMET_SCHEDULER_EXTERNAL_HOST`: the host executors reach this scheduler at, which they +//! report task status to (default `localhost`) //! - `COMET_SCHEDULER_PORT`: Ballista's gRPC port, for executors (default `50050`) //! - `COMET_SCHEDULER_FLIGHT_PORT`: the Flight port, for drivers (default `50060`) //! - `COMET_SCHEDULER_RESULT_RETENTION_SECONDS`: how long a query's results stay readable after //! the query ends, whether or not a driver has read them (default `300`). `0` keeps them. +//! - `COMET_GRPC_MAX_MESSAGE_SIZE`: the largest gRPC message, in bytes, the scheduler sends or +//! accepts, such as a task with many partitions of a scan (default: Ballista's 16 MiB) +//! - `COMET_SCHEDULER_EVENT_LOG_DIR`: a directory to write one event log per query to, as +//! Ballista's scheduler does: the query's plan, a record for each task, and each stage's rows +//! and compute time (default: no event logs) Review Comment: Could the docs describe the per-task timeline as best effort? Ballista drops timeline records when the writer queue fills, so a completed job can have fewer records than tasks. It would also help to say that event-log files remain in this directory until removed. The result-retention setting above governs executor result data. [Event-log writer](https://github.com/apache/datafusion-ballista/blob/55.0.0/ballista/history/src/writer.rs#L63-L85) -- 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]
