[GitHub] [arrow-datafusion] houqp commented on a change in pull request #1983: Introduce StageManager for managing tasks stage by stage

[GitBox <gi...@apache.org>]

houqp commented on a change in pull request #1983:
URL: https://github.com/apache/arrow-datafusion/pull/1983#discussion_r824416389



##########
File path: ballista/rust/scheduler/src/scheduler_server/event_loop.rs
##########
@@ -50,32 +47,53 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan>
     pub fn new(
         state: Arc<SchedulerState<T, U>>,
         executors_client: ExecutorsClient,
+        is_test: bool,
     ) -> Self {
         Self {
             state,
             executors_client,
+            is_test,
         }
     }
 
-    async fn offer_resources(
-        &self,
-        job_id: String,
-    ) -> Result<Option<SchedulerServerEvent>> {
-        let mut available_executors = self.state.get_available_executors_data();
+    async fn offer_resources(&self, n: u32) -> Result<Option<SchedulerServerEvent>> {
+        let mut available_executors = if self.is_test {

Review comment:
       IMO, setting flags specific for tests in application logic is usually not an anti-pattern. It makes the code harder to read and adds unnecessary runtime overhead. If I understand it correctly, the main problem is we don't have executor heartbeats in tests causing `get_alive_executors` to return an empty set? If so, I would recommend adding a helper method to set artificial executor heartbeats directly from tests so we can keep the core logic agnostics to tests.




-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org