andygrove commented on code in PR #2787:
URL:
https://github.com/apache/datafusion-ballista/pull/2787#discussion_r4493922107
##########
ballista/history/src/writer.rs:
##########
@@ -142,6 +142,18 @@ impl EventLogWriter {
}
}
+/// Check that event logs can be written under `log_dir`: create it if it is
+/// missing, then create and remove a file in it.
+///
+/// [`EventLogWriter`] only logs a warning when it cannot write, so a caller
+/// that needs the logs should check the directory before starting the writer.
+pub async fn check_log_dir(log_dir: &Path) -> std::io::Result<()> {
+ tokio::fs::create_dir_all(log_dir).await?;
+ let probe = log_dir.join(format!(".write-check-{}", std::process::id()));
Review Comment:
One concern with the probe name. `.write-check-{pid}` is the same for every
scheduler that runs as pid 1 in a container. The history server guide describes
several schedulers writing into one directory, so two of them can start against
the same volume. One can then remove the other's probe and fail with `NotFound`
on `remove_file`. That would stop a healthy scheduler at startup with a
misleading "cannot write event logs" error.
Would you consider a random suffix, or treating `NotFound` from
`remove_file` as success? A small test that runs `check_log_dir` concurrently
on one directory would pin it down.
##########
ballista/scheduler/src/scheduler_server/mod.rs:
##########
@@ -310,6 +310,18 @@ impl<T: 'static + AsLogicalPlan, U: 'static +
AsExecutionPlan> SchedulerServer<T
/// Initializes the scheduler and starts background tasks.
pub async fn init(&mut self) -> Result<()> {
+ // The event-log writer only warns when it cannot write, so fail here
+ // instead of running without the logs that were asked for.
+ #[cfg(feature = "rest-api")]
+ if let Some(dir) = &self.config.event_log_dir {
+ ballista_history::writer::check_log_dir(std::path::Path::new(dir))
Review Comment:
`init` can now return an error that it did not return before. Embedders that
call it with a misconfigured `event_log_dir` will start failing after the
upgrade. The signature is unchanged so I don't think this needs the
`api-change` label, but could you add a short note to
`docs/source/upgrading/55.0.0.md`? That is where library users will look.
--
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]
