andygrove commented on code in PR #7420:
URL: https://github.com/apache/datafusion-comet/pull/7420#discussion_r4447173300
##########
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:
Good catch, thanks. The scheduler now sets `disable_rest_api: true`, so
`rest-api` only brings in the event-log writer. I added
`writes_event_logs_without_serving_the_rest_api` to `tests/cluster.rs`. It
starts a cluster through `Cluster::start_with` with
`COMET_SCHEDULER_EVENT_LOG_DIR` set, runs a query, waits for a finished
`.eventlog`, and asserts that `GET /api/jobs` on the scheduler port returns
404. With `disable_rest_api: false` the same test fails with a 200. Fixed in
94010d26d.
##########
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:
Done in 94010d26d. The scheduler's doc now covers only Ballista's
scheduler-executor gRPC service and says to set the variable on the executors
too. It notes that the drivers' Flight service has no limit, and that shuffle
reads between executors keep `ballista.client.grpc_max_message_size` (16 MiB by
default), which this variable doesn't change. The executor's doc lists where it
does apply: the connections with the scheduler and its Flight service.
##########
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:
Done in 94010d26d. The doc now calls it a budget, says `0` means no limit,
and explains that each task gets its own pool of the budget divided by
`COMET_EXECUTOR_TASKS`, times the slots it takes, even when it runs alone. It
also gives the default: 70% of the smaller of the cgroup limit and host memory,
or no limit if neither is detected.
##########
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:
Done in 94010d26d. The doc now says the task records are best effort and may
be dropped when the writer falls behind, so a log can have fewer records than
the query had tasks. It also says logs stay in the directory until something
removes them, and that the result-retention setting applies to query results,
not to logs.
--
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]
