From 2a76dc25447ee260139fcb903ee88b91658b0271 Mon Sep 17 00:00:00 2001 From: lostluck <13907733+lostluck@users.noreply.github.com> Date: Thu, 18 Apr 2024 14:28:58 -0700 Subject: [PATCH 1/4] Stabilize additional teststream cases. --- .../prism/internal/engine/elementmanager.go | 26 ++++++- .../prism/internal/engine/engine_test.go | 5 ++ .../prism/internal/engine/teststream.go | 49 +++++++++++- .../runners/prism/internal/worker/bundle.go | 5 +- .../test/integration/primitives/teststream.go | 74 +++++++++++++++++++ .../integration/primitives/teststream_test.go | 20 +++++ sdks/go/test/integration/primitives/timers.go | 2 +- 7 files changed, 172 insertions(+), 9 deletions(-) diff --git a/sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go b/sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go index 28ea75ac9e52f..ba0ab6f13d272 100644 --- a/sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go +++ b/sdks/go/pkg/beam/runners/prism/internal/engine/elementmanager.go @@ -153,7 +153,8 @@ type Config struct { type ElementManager struct { config Config - stages map[string]*stageState // The state for each stage. + impulses set[string] // List of impulse stages. + stages map[string]*stageState // The state for each stage. consumers map[string][]string // Map from pcollectionID to stageIDs that consumes them as primary input. sideConsumers map[string][]LinkID // Map from pcollectionID to the stage+transform+input that consumes them as side input. @@ -254,6 +255,14 @@ func (em *ElementManager) Impulse(stageID string) { em.addPending(count) } refreshes := stage.updateWatermarks(em) + + // Since impulses are synthetic, we need to simulate them properly + // if a pipeline is only test stream driven. + if em.impulses == nil { + em.impulses = refreshes + } else { + em.impulses.merge(refreshes) + } em.addRefreshes(refreshes) } @@ -286,6 +295,13 @@ func (em *ElementManager) Bundles(ctx context.Context, nextBundID func() string) // Watermark evaluation goroutine. go func() { defer close(runStageCh) + + // If we have a test stream, clear out existing refreshes, so the test stream can + // insert any elements it needs. + if em.testStreamHandler != nil { + em.watermarkRefreshes = singleSet(em.testStreamHandler.ID) + } + for { em.refreshCond.L.Lock() // If there are no watermark refreshes available, we wait until there are. @@ -370,7 +386,13 @@ func (em *ElementManager) checkForQuiescence(advanced set[string]) { nextEvent.Execute(em) // Decrement pending for the event being processed. em.addPending(-1) - return + // If there are refreshes scheduled, then test stream permitted execution to continue. + // Note: it's a prism bug if test stream never causes a refresh to occur for a given event. + // It's not correct to move to the next event if no refreshes would occur. + if len(em.watermarkRefreshes) > 0 { + return + } + // If there are no refreshes, then there's no mechanism to make progress, so it's time to fast fail. } v := em.livePending.Load() diff --git a/sdks/go/pkg/beam/runners/prism/internal/engine/engine_test.go b/sdks/go/pkg/beam/runners/prism/internal/engine/engine_test.go index 04269e3dd6af2..0c042d731d6ac 100644 --- a/sdks/go/pkg/beam/runners/prism/internal/engine/engine_test.go +++ b/sdks/go/pkg/beam/runners/prism/internal/engine/engine_test.go @@ -186,6 +186,11 @@ func TestTestStream(t *testing.T) { {pipeline: primitives.TestStreamTwoFloat64Sequences}, {pipeline: primitives.TestStreamTwoInt64Sequences}, {pipeline: primitives.TestStreamTwoUserTypeSequences}, + + {pipeline: primitives.TestStreamSimple}, + {pipeline: primitives.TestStreamSimple_InfinityDefault}, + {pipeline: primitives.TestStreamToGBK}, + {pipeline: primitives.TestStreamTimersEventTime}, } configs := []struct { diff --git a/sdks/go/pkg/beam/runners/prism/internal/engine/teststream.go b/sdks/go/pkg/beam/runners/prism/internal/engine/teststream.go index c0a0ff8ebe7d3..f0350064d526a 100644 --- a/sdks/go/pkg/beam/runners/prism/internal/engine/teststream.go +++ b/sdks/go/pkg/beam/runners/prism/internal/engine/teststream.go @@ -16,6 +16,7 @@ package engine import ( + "container/heap" "time" "github.com/apache/beam/sdks/v2/go/pkg/beam/core/graph/mtime" @@ -46,13 +47,15 @@ type testStreamHandler struct { tagState map[string]tagState // Map from event tag to related outputs. - completed bool // indicates that no further test stream events exist, and all watermarks are advanced to infinity. Used to send the final event, once. + currentHold mtime.Time // indicates if the default watermark hold has been lifted. + completed bool // indicates that no further test stream events exist, and all watermarks are advanced to infinity. Used to send the final event, once. } func makeTestStreamHandler(id string) *testStreamHandler { return &testStreamHandler{ - ID: id, - tagState: map[string]tagState{}, + ID: id, + tagState: map[string]tagState{}, + currentHold: mtime.MinTimestamp, } } @@ -124,6 +127,35 @@ func (ts *testStreamHandler) NextEvent() tsEvent { return ev } +// UpdateHold restrains the watermark based on upcoming elements in the test stream queue +// This uses the element manager's normal hold mechnanisms to avoid premature pipeline termination, +// when there are still remaining events to process. +func (ts *testStreamHandler) UpdateHold(em *ElementManager, newHold mtime.Time) { + if ts == nil { + return + } + + ss := em.stages[ts.ID] + ss.mu.Lock() + defer ss.mu.Unlock() + + if ss.watermarkHoldsCounts[ts.currentHold] > 0 { + heap.Pop(&ss.watermarkHoldHeap) + ss.watermarkHoldsCounts[ts.currentHold] = ss.watermarkHoldsCounts[ts.currentHold] - 1 + } + ts.currentHold = newHold + heap.Push(&ss.watermarkHoldHeap, ts.currentHold) + ss.watermarkHoldsCounts[ts.currentHold] = 1 + + // kick the TestStream and Impulse stages too. + kick := singleSet(ts.ID) + kick.merge(em.impulses) + + // This executes under the refreshCond lock, so we can't call em.addRefreshes. + em.watermarkRefreshes.merge(kick) + em.refreshCond.Broadcast() +} + // TestStreamElement wraps the provided bytes and timestamp for ingestion and use. type TestStreamElement struct { Encoded []byte @@ -195,6 +227,8 @@ func (ev tsWatermarkEvent) Execute(em *ElementManager) { ss.updateUpstreamWatermark(ss.inputID, t.watermark) em.watermarkRefreshes.insert(sID) } + // Clear the default hold after the inserts have occured. + em.testStreamHandler.UpdateHold(em, t.watermark) } // tsProcessingTimeEvent implements advancing the synthetic processing time. @@ -215,7 +249,7 @@ type tsFinalEvent struct { } func (ev tsFinalEvent) Execute(em *ElementManager) { - em.addPending(1) // We subtrack a pending after event execution, so add one now. + em.testStreamHandler.UpdateHold(em, mtime.MaxTimestamp) ss := em.stages[ev.stageID] kickSet := ss.updateWatermarks(em) kickSet.insert(ev.stageID) @@ -242,6 +276,13 @@ var ( func (tsi *testStreamImpl) initHandler(id string) { if tsi.em.testStreamHandler == nil { tsi.em.testStreamHandler = makeTestStreamHandler(id) + + ss := tsi.em.stages[id] + tsi.em.addPending(1) // We subtrack a pending after event execution, so add one now for the final event to avoid a race condition. + + // Arrest the watermark initially to prevent terminal advancement. + heap.Push(&ss.watermarkHoldHeap, tsi.em.testStreamHandler.currentHold) + ss.watermarkHoldsCounts[tsi.em.testStreamHandler.currentHold] = 1 } } diff --git a/sdks/go/pkg/beam/runners/prism/internal/worker/bundle.go b/sdks/go/pkg/beam/runners/prism/internal/worker/bundle.go index 35d23e7024a5c..842c5fdfc19de 100644 --- a/sdks/go/pkg/beam/runners/prism/internal/worker/bundle.go +++ b/sdks/go/pkg/beam/runners/prism/internal/worker/bundle.go @@ -73,9 +73,10 @@ type B struct { func (b *B) Init() { // We need to see final data and timer signals that match the number of // outputs the stage this bundle executes posesses - b.dataSema.Store(int32(b.OutputCount + len(b.HasTimers))) + outCap := int32(b.OutputCount + len(b.HasTimers)) + b.dataSema.Store(outCap) b.DataWait = make(chan struct{}) - if b.OutputCount == 0 { + if outCap == 0 { close(b.DataWait) // Can happen if there are no outputs for the bundle. } b.Resp = make(chan *fnpb.ProcessBundleResponse, 1) diff --git a/sdks/go/test/integration/primitives/teststream.go b/sdks/go/test/integration/primitives/teststream.go index c8ba9b565c0f2..5ec619fe608cd 100644 --- a/sdks/go/test/integration/primitives/teststream.go +++ b/sdks/go/test/integration/primitives/teststream.go @@ -16,7 +16,10 @@ package primitives import ( + "fmt" + "github.com/apache/beam/sdks/v2/go/pkg/beam" + "github.com/apache/beam/sdks/v2/go/pkg/beam/register" "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/passert" "github.com/apache/beam/sdks/v2/go/pkg/beam/testing/teststream" ) @@ -172,3 +175,74 @@ func TestStreamInt16Sequence(s beam.Scope) { passert.Count(s, col, "teststream int15", 3) passert.EqualsList(s, col, ele) } + +// panicIfNot42 panics if the value is not 42. +func panicIfNot42(v int) { + if v != 42 { + panic(fmt.Sprintf("got %v, want 42", v)) + } +} + +// dropKeyEmitValues panics if the value is not 42. +func dropKeyEmitValues(_ int, vs func(*int) bool, emit func(int)) { + var v int + for vs(&v) { + emit(v) + } +} + +func init() { + register.Function1x0(panicIfNot42) + register.Function3x0(dropKeyEmitValues) +} + +// TestStreamSimple is a trivial pipeline where teststream sends +// a single element to a DoFn that checks that it's received the value. +// Intended for runner validation. +func TestStreamSimple(s beam.Scope) { + con := teststream.NewConfig() + ele := []int{42} + con.AddElementList(100, ele) + con.AdvanceWatermarkToInfinity() + + col := teststream.Create(s, con) + beam.ParDo0(s, panicIfNot42, col) +} + +// TestStreamSimple_InfinityDefault is the same trivial pipeline that +// validates that the watermark is automatically advanced to infinity +// even when the user doesn't set it. +// Intended for runner validation. +func TestStreamSimple_InfinityDefault(s beam.Scope) { + con := teststream.NewConfig() + ele := []int{42} + con.AddElementList(100, ele) + + col := teststream.Create(s, con) + beam.ParDo0(s, panicIfNot42, col) +} + +// TestStreamToGBK is a trivial pipeline where teststream sends +// a single element to a GBK. +func TestStreamToGBK(s beam.Scope) { + con := teststream.NewConfig() + ele := []int{42} + con.AddElementList(100, ele) + con.AdvanceWatermarkToInfinity() + + col := teststream.Create(s, con) + keyed := beam.AddFixedKey(s, col) + gbk := beam.GroupByKey(s, keyed) + dropped := beam.ParDo(s, dropKeyEmitValues, gbk) + beam.ParDo0(s, panicIfNot42, dropped) +} + +// TestStreamTimersEventTime validates event time timers in a test stream "driven" pipeline. +func TestStreamTimersEventTime(s beam.Scope) { + timersEventTimePipelineBuilder(func(s beam.Scope) beam.PCollection { + c := teststream.NewConfig() + c.AddElements(123456, []byte{42}) + c.AdvanceWatermarkToInfinity() + return teststream.Create(s, c) + })(s) +} diff --git a/sdks/go/test/integration/primitives/teststream_test.go b/sdks/go/test/integration/primitives/teststream_test.go index b0144f148cb0e..14df6bf9b95c7 100644 --- a/sdks/go/test/integration/primitives/teststream_test.go +++ b/sdks/go/test/integration/primitives/teststream_test.go @@ -71,3 +71,23 @@ func TestTestStreamTwoUserTypeSequences(t *testing.T) { integration.CheckFilters(t) ptest.BuildAndRun(t, TestStreamTwoUserTypeSequences) } + +func TestTestStreamSimple(t *testing.T) { + integration.CheckFilters(t) + ptest.BuildAndRun(t, TestStreamSimple) +} + +func TestTestStreamTestStreamSimple_InfinityDefault(t *testing.T) { + integration.CheckFilters(t) + ptest.BuildAndRun(t, TestStreamSimple_InfinityDefault) +} + +func TestTestStreamToGBK(t *testing.T) { + integration.CheckFilters(t) + ptest.BuildAndRun(t, TestStreamToGBK) +} + +func TestTestStreamTimersEventTimeTestStream(t *testing.T) { + integration.CheckFilters(t) + ptest.BuildAndRun(t, TestStreamTimersEventTime) +} diff --git a/sdks/go/test/integration/primitives/timers.go b/sdks/go/test/integration/primitives/timers.go index b4443296eccec..4b7f6e9765f0a 100644 --- a/sdks/go/test/integration/primitives/timers.go +++ b/sdks/go/test/integration/primitives/timers.go @@ -93,7 +93,7 @@ func (fn *eventTimeFn) OnTimer(ctx context.Context, ts beam.EventTime, sp state. } } -// TimersEventTime takes in an impulse transform and produces a pipeline construction +// timersEventTimePipelineBuilder takes in an impulse transform and produces a pipeline construction // function that validates EventTime timers. // // The impulse is provided outside to swap between a bounded impulse, and From f146ec241b4ac14c05dce970f59f4ab2946908f2 Mon Sep 17 00:00:00 2001 From: Robert Burke Date: Mon, 22 Apr 2024 10:09:52 -0700 Subject: [PATCH 2/4] Update sdks/go/test/integration/primitives/teststream_test.go Co-authored-by: Ritesh Ghorse --- sdks/go/test/integration/primitives/teststream_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdks/go/test/integration/primitives/teststream_test.go b/sdks/go/test/integration/primitives/teststream_test.go index 14df6bf9b95c7..4ec5527d774ac 100644 --- a/sdks/go/test/integration/primitives/teststream_test.go +++ b/sdks/go/test/integration/primitives/teststream_test.go @@ -87,7 +87,7 @@ func TestTestStreamToGBK(t *testing.T) { ptest.BuildAndRun(t, TestStreamToGBK) } -func TestTestStreamTimersEventTimeTestStream(t *testing.T) { +func TestTestStreamTimersEventTime(t *testing.T) { integration.CheckFilters(t) ptest.BuildAndRun(t, TestStreamTimersEventTime) } From 366d230c62b92ecc87f1de1db10b82f25c089fe5 Mon Sep 17 00:00:00 2001 From: Robert Burke Date: Mon, 22 Apr 2024 10:09:58 -0700 Subject: [PATCH 3/4] Update sdks/go/test/integration/primitives/teststream.go Co-authored-by: Ritesh Ghorse --- sdks/go/test/integration/primitives/teststream.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdks/go/test/integration/primitives/teststream.go b/sdks/go/test/integration/primitives/teststream.go index 5ec619fe608cd..404817fd47034 100644 --- a/sdks/go/test/integration/primitives/teststream.go +++ b/sdks/go/test/integration/primitives/teststream.go @@ -183,7 +183,7 @@ func panicIfNot42(v int) { } } -// dropKeyEmitValues panics if the value is not 42. +// dropKeyEmitValues drops the key and emits the value. func dropKeyEmitValues(_ int, vs func(*int) bool, emit func(int)) { var v int for vs(&v) { From d17422e1be85b1041bfbea6cd3f6bad6a7e06524 Mon Sep 17 00:00:00 2001 From: Robert Burke Date: Mon, 22 Apr 2024 10:13:02 -0700 Subject: [PATCH 4/4] Update sdks/go/test/integration/primitives/teststream_test.go Co-authored-by: Ritesh Ghorse --- sdks/go/test/integration/primitives/teststream_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdks/go/test/integration/primitives/teststream_test.go b/sdks/go/test/integration/primitives/teststream_test.go index 4ec5527d774ac..3bd1a7d4de45b 100644 --- a/sdks/go/test/integration/primitives/teststream_test.go +++ b/sdks/go/test/integration/primitives/teststream_test.go @@ -77,7 +77,7 @@ func TestTestStreamSimple(t *testing.T) { ptest.BuildAndRun(t, TestStreamSimple) } -func TestTestStreamTestStreamSimple_InfinityDefault(t *testing.T) { +func TestTestStreamSimple_InfinityDefault(t *testing.T) { integration.CheckFilters(t) ptest.BuildAndRun(t, TestStreamSimple_InfinityDefault) }