Yicong-Huang commented on code in PR #4444:
URL: https://github.com/apache/texera/pull/4444#discussion_r3144037142
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/WorkflowExecutionCoordinator.scala:
##########
@@ -21,25 +21,27 @@ package
org.apache.texera.amber.engine.architecture.scheduling
import com.twitter.util.Future
import com.typesafe.scalalogging.LazyLogging
+import org.apache.texera.amber.core.virtualidentity.OperatorIdentity
import org.apache.texera.amber.core.workflow.{GlobalPortIdentity, PhysicalLink}
import org.apache.texera.amber.engine.architecture.common.{
AkkaActorRefMappingService,
AkkaActorService
}
-import org.apache.texera.amber.engine.architecture.controller.ControllerConfig
+import
org.apache.texera.amber.engine.architecture.controller.{ControllerConfig,
WorkflowScheduler}
import
org.apache.texera.amber.engine.architecture.controller.execution.WorkflowExecution
import org.apache.texera.amber.engine.common.rpc.AsyncRPCClient
import scala.collection.mutable
class WorkflowExecutionCoordinator(
- getNextRegions: () => Set[Region],
+ workflowScheduler: WorkflowScheduler,
Review Comment:
Understood. It was not documented well.
The `Scheduler` produces a `Schedule`, which is a ranked or ordered list of
`Region`s.
`WorkflowExecutionCoordinator` coordinates workflow execution. Whenever the
current set of regions finishes execution, it pulls the next set of regions by
calling `getNextRegions`. This design is intentional: the coordinator should
not depend on a fixed, fully materialized schedule, but expects an iterator of
ranked regions.
This is not unused design. I had an unmerged branch that relied on this
behavior. In that branch, we submit a workflow for execution, and the scheduler
produces a schedule, say S1. During workflow execution, if we add a new
operator or delete an operator, the scheduler produces a new schedule, say S2.
Both S1 and S2 are immutable data classes. When WorkflowExecutionCoordinator
finishes executing its current regions, it calls getNextRegions; at that point,
the next regions are computed by taking both S1 and S2 into account. This
supports modifying workflow during execution. We did not merge it because it is
an experimental feature.
If this PR does not require changing this design, I’d prefer to leave it as
is for now. Thanks for taking a 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]
