andygrove commented on code in PR #7420:
URL: https://github.com/apache/datafusion-comet/pull/7420#discussion_r4494680845
##########
native/native-executors/src/bin/comet-scheduler.rs:
##########
@@ -79,3 +120,12 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
}
Ok(())
}
+
+/// Creates `dir` if it doesn't exist, and checks that the scheduler can
create files in it: a
+/// directory that already exists, such as a volume mounted with another
owner, may not allow it.
+fn check_writable(dir: &Path) -> std::io::Result<()> {
+ std::fs::create_dir_all(dir)?;
+ let probe = dir.join(format!(".comet-scheduler-{}", std::process::id()));
Review Comment:
Good point. The probe name now has the time in nanoseconds as well as the
PID, so two schedulers sharing a log volume no longer collide. Fixed in
10defc377.
##########
native/native-executors/tests/cluster.rs:
##########
@@ -166,6 +170,24 @@ fn free_port() -> u16 {
.port()
}
+/// The HTTP status of a GET of `path` on a local port.
+fn http_status(port: u16, path: &str) -> u16 {
+ let mut stream = TcpStream::connect(("127.0.0.1", port)).unwrap();
Review Comment:
Done in 10defc377: `http_status` sets a 10 second read timeout.
##########
native/native-executors/src/bin/comet-scheduler.rs:
##########
@@ -20,12 +20,35 @@
//!
//! 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 message, in bytes, of
Ballista's gRPC service
+//! between the scheduler and its executors, such as a task with many
partitions of a scan
+//! (default: Ballista's 16 MiB). Set it on the executors as well, for their
side of the same
+//! messages. The Flight service for drivers has no limit. Shuffle reads
between executors keep
+//! the limit of the query's `ballista.client.grpc_max_message_size` (16 MiB
by default), which
+//! this does not change.
+//! - `COMET_SCHEDULER_EVENT_LOG_DIR`: a directory to write one event log per
query to, as
+//! Ballista's scheduler does (default: no event logs). The scheduler
creates it if needed and
+//! doesn't start if it can't write there. A log has the query's plan and a
record for each
+//! stage and task, with the task's times and the rows it wrote. Ballista
sees a stage's Comet
Review Comment:
No. The doc now says "the log" sees a stage's Comet block as one operator,
without naming Ballista. Done in 10defc377.
##########
native/native-executors/src/bin/comet-scheduler.rs:
##########
@@ -43,17 +66,35 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
init_logging();
let mut config = SchedulerConfig {
override_physical_codec: Some(Arc::new(CometPhysicalCodec::default())),
+ // Compiling the event-log writer compiles the REST API in with it, on
the gRPC port.
+ disable_rest_api: true,
..Default::default()
};
if let Some(host) = env_var("COMET_SCHEDULER_BIND_HOST")? {
config.bind_host = host;
}
+ if let Some(host) = env_var("COMET_SCHEDULER_EXTERNAL_HOST")? {
+ config.external_host = host;
+ }
+ if let Some(size) = env_var("COMET_GRPC_MAX_MESSAGE_SIZE")? {
Review Comment:
Yes. The scheduler is a gRPC server for the executors and also a client of
theirs, and Ballista has a separate `grpc_client_max_message_size` for that
client. An executor has one pair of settings, which covers both its server and
its connection to the scheduler. I added a comment saying so in 10defc377.
--
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]
