Xiao-zhen-Liu commented on code in PR #5720:
URL: https://github.com/apache/texera/pull/5720#discussion_r3450003812
##########
amber/src/test/scala/org/apache/texera/amber/engine/architecture/scheduling/CostBasedScheduleGeneratorSpec.scala:
##########
@@ -515,4 +516,40 @@ class CostBasedScheduleGeneratorSpec extends AnyFlatSpec
with MockFactory {
)
}
+ "CostBasedScheduleGenerator.effectiveExecutionMode" should
Review Comment:
Both helper branches covered well. Gap: nothing tests that createRegionDAG
actually calls the helper. Fine to defer to the loop integration tests if
that's the plan.
##########
amber/src/main/scala/org/apache/texera/amber/engine/architecture/scheduling/CostBasedScheduleGenerator.scala:
##########
@@ -304,7 +321,14 @@ class CostBasedScheduleGenerator(
*/
private def createRegionDAG(): DirectedAcyclicGraph[Region, RegionLink] = {
val searchResultFuture: Future[SearchResult] = Future {
Review Comment:
This inline comment repeats the helper's scaladoc and the field doc — three
copies that'll drift. Drop it; the named helper is self-explanatory.
##########
common/workflow-core/src/main/scala/org/apache/texera/amber/core/workflow/PhysicalOp.scala:
##########
@@ -198,6 +198,11 @@ case class PhysicalOp(
// schema propagation function
propagateSchema: SchemaPropagationFunc = SchemaPropagationFunc(schemas =>
schemas),
isOneToManyOp: Boolean = false,
+ // Whether this operator can only run correctly under a fully-materialized
+ // schedule (e.g. a loop operator, whose back-edge is a cross-region
+ // materialized state channel). The schedule generator forces materialized
+ // region boundaries when any operator sets this. Default false.
+ requiresMaterializedExecution: Boolean = false,
Review Comment:
Doc says it forces materialized "region boundaries," but the consumer
materializes every link in the plan. One op with this flag turns the whole
workflow fully-materialized and kills pipelining everywhere. Is whole-plan
materialization really what loops need, or just their own boundaries? Either
way, reword the doc to match.
--
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]
