andygrove commented on PR #2431: URL: https://github.com/apache/datafusion-ballista/pull/2431#issuecomment-5590807168
Nice cleanup of the internals, the source and trigger split reads well and the notify tests are thorough. The one thing I would like to work through before this lands is dropping the periodic pass entirely. The old polling loop had a comment explaining why it polled rather than watched: history servers are usually pointed at a directory on a shared or network filesystem, written by schedulers on other hosts, and inotify does not see remote writes. With `OnceTrigger` as the only scan trigger, that deployment gets a job list frozen at startup with no error to explain it. Could we keep both? Watch for the low latency path, plus a periodic trigger as a backstop, with `--update-interval-seconds` retained to tune it and `0` meaning watch-only. A longer default than 10s makes sense now that notify carries the common case. That also covers the coarse-backend case your own comment in `trigger.rs` mentions, where a missed removal currently has no next full scan to wait for. Two smaller things. `HistoryStore::new` does not actually call `load_index`, so the rustdoc on it and the comment in `history_server.rs` about a synchronous initial walk are describing something that does not happen, and the store is empty until the scan loop runs. And `docs/source/user-guide/history-server.md` still documents the interval flag, the stat-per-file cost, and deletion reconciliation on the next pass, so that section needs updating with this. Since `history` is a public module, `load`, `refresh`, `RefreshStats` and `spawn_refresh_task` going away is a breaking change for anyone embedding the scheduler crate. Could you add the `api change` label and a short entry in `docs/source/upgrading/55.0.0.md`? Several of those could stay as deprecated wrappers too, which would soften it. Last one, the rationale sells the traits as a seam for object store and remote backends, but they are `pub(crate)` and there is no constructor taking them, so an embedder cannot plug one in yet. Worth either exposing them or trimming that from the description. -- 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]
