andygrove commented on code in PR #1891:
URL:
https://github.com/apache/datafusion-ballista/pull/1891#discussion_r3482864543
##########
ballista/scheduler/src/scheduler_server/mod.rs:
##########
@@ -85,9 +88,20 @@ pub struct SchedulerServer<T: 'static + AsLogicalPlan, U:
'static + AsExecutionP
query_stage_scheduler: Arc<QueryStageScheduler<T, U>>,
/// Scheduler configuration.
config: Arc<SchedulerConfig>,
+ /// Broadcast sender for job state change notifications.
+ ///
+ /// Subscribers can receive notifications when jobs change state by calling
+ /// `subscribe_job_updates()`.
+ job_state_sender: broadcast::Sender<job_state_event::JobStateEvent>,
}
impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan>
SchedulerServer<T, U> {
+ /// Default capacity for the job state broadcast channel.
+ ///
+ /// This determines how many job state events can be buffered before
+ /// slow receivers start lagging behind.
+ const JOB_STATE_CHANNEL_CAPACITY: usize = 256;
Review Comment:
The 256 capacity is a sensible default. For a scheduler with high job
throughput and a slow or briefly stalled subscriber, the buffer could fill and
subscribers would start seeing `Lagged`. Would it be worth pulling this into
`SchedulerConfig` so operators with heavier job rates can size it, keeping 256
as the default? Different deployments run very different workloads, so a fixed
buffer may not fit everyone.
##########
ballista/scheduler/src/scheduler_server/query_stage_scheduler.rs:
##########
@@ -211,6 +236,10 @@ impl<T: 'static + AsLogicalPlan, U: 'static +
AsExecutionPlan>
.record_completed(&job_id, queued_at, completed_at);
info!("Job finished successfully: [{job_id}]");
+
+ // Broadcast job completed state
+ self.broadcast_job_state(JobStateEvent::completed(&job_id));
Review Comment:
One thing I noticed about ordering. The `Completed` event is broadcast here
just before `succeed_job` runs, and the same pattern holds for `queued` (before
`queue_job`), `failed`, and `cancelled`. The `Running` event, by contrast,
fires after `submit_job` succeeds. So most events are emitted before the
corresponding state transition is committed, which means a subscriber that
reacts by querying the scheduler could briefly see stale state. There's also
the case where `succeed_job` or `cancel_job` returns an error after the event
already went out, so a subscriber would have been told the job completed when
the commit actually failed. Would it make sense to broadcast after the state
transition succeeds, so events line up with committed state and with how
`Running` already behaves?
--
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]
