[#29772][Go SDK] Add EventTime Timer tests. #29829

lostluck · 2023-12-19T23:40:37Z

Add event time timer validation tests.

Execute both with bounded, and unbounded modes incase runners execute them differently (namely dataflow).

Fix bug in handling MapWindows, which couldn't map from Global Windows to GlobalWindows properly due to a quirk in the SDK's Window Coder + KV handling.

Pre-Work for #29772

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
Update CHANGES.md with noteworthy changes.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.

lostluck · 2023-12-19T23:41:28Z

Currently triggering all the post commits, so I can properly add filtering for failures.

codecov · 2023-12-19T23:46:45Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (1986517) 38.23% compared to head (b8b5350) 38.24%.
Report is 31 commits behind head on master.

❗ Current head b8b5350 differs from pull request most recent head 1fe6f00. Consider uploading reports for the commit 1fe6f00 to get more accurate results

Files	Patch %	Lines
sdks/go/pkg/beam/core/runtime/exec/window.go	63.63%	3 Missing and 1 partial ⚠️
sdks/go/pkg/beam/core/runtime/exec/translate.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #29829      +/-   ##
==========================================
+ Coverage   38.23%   38.24%   +0.01%     
==========================================
  Files         696      696              
  Lines      101878   101883       +5     
==========================================
+ Hits        38952    38969      +17     
+ Misses      61309    61299      -10     
+ Partials     1617     1615       -2

Flag	Coverage Δ
go	`53.97% <58.33%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2023-12-20T02:34:36Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

lostluck · 2023-12-20T05:24:35Z

Yeah, just Dataflow appears to support timers. The others fail in various ways.

github-actions · 2023-12-20T08:06:19Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @jrmccluskey for label go.
R: @damccorm for label build.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

lostluck · 2023-12-20T23:24:42Z

R: @damondouglas

github-actions · 2023-12-20T23:25:44Z

Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control

lostluck · 2023-12-27T19:17:57Z

Friendly ping for a review here.

sdks/go/test/integration/primitives/timers.go

damondouglas

FYI sdks/go/test/integration/integration.go says at the top of the file:

// Running integration tests can be done with a go test call with any flags that
// are required by the test pipelines, such as --runner or --endpoint.
// Example:
//
//	go test -v ./sdks/go/test/integration/... --runner=portable --endpoint=localhost:8099

which results in the error pattern ./sdks/go/test/integration/...: directory prefix sdks/go/test/integration does not contain main module or its selected dependencies. This makes sense since there's no go.mod at the root of the repos. I was able to execute this PR's branch in an ephemeral Google Cloud console using:

cd sdks
go test -v ./go/test/integration/... --runner=portable --endpoint=localhost:8099

However, I received the error:

2023/12/28 17:40:50 Cross-compiling /home/damondouglas/timerIntegrationTests/sdks/go/test/integration/wordcount/wordcount_test.go with GOOS=linux GOARCH=amd64 CGO_ENABLED=0 as /tmp/worker-1-1703785250974444350
    wordcount_test.go:113: WordCount("foo") failed:     connecting to job service
        failed to dial server at localhost:8099
                caused by:
        context deadline exceeded
--- FAIL: TestWordCount (188.39s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x1847fbc]

goroutine 23 [running]:
testing.tRunner.func1.2({0x1b951a0, 0x4abc560})
        /usr/local/go/src/testing/testing.go:1545 +0x238
testing.tRunner.func1()
        /usr/local/go/src/testing/testing.go:1548 +0x397
panic({0x1b951a0?, 0x4abc560?})
        /usr/local/go/src/runtime/panic.go:914 +0x21f
github.com/apache/beam/sdks/v2/go/pkg/beam/runners/universal/runnerlib.universalPipelineResult.Metrics(...)
        /timerIntegrationTests/sdks/go/pkg/beam/runners/universal/runnerlib/execute.go:164
github.com/apache/beam/sdks/v2/go/test/integration/wordcount.TestWordCount(0xc000500ea0)
        /timerIntegrationTests/sdks/go/test/integration/wordcount/wordcount_test.go:116 +0x4b7
testing.tRunner(0xc000500ea0, 0x2582458)
        /usr/local/go/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 1
        /usr/local/go/src/testing/testing.go:1648 +0x3ad
FAIL    github.com/apache/beam/sdks/v2/go/test/integration/wordcount    188.507s

sdks/go/test/integration/primitives/timers.go

damondouglas · 2023-12-28T17:55:46Z

sdks/go/test/integration/primitives/timers.go

+// TimersEventTime takes in an impulse transform and then validates
+// event time timer execution.
+//
+// The impulse is provided outside to swap between a bounded impulse, and
+// an unbounded one, because the Go SDK uses that to determine if a pipeline
+// is "streaming" or not. This matters at least for executions on Dataflow.
+//
+// Regardless,the pipelines should pass.


May we consider the following? (Blame the comment lines alignment on the GitHub comment editor).

// Validator is a func that validates a Pipeline. type Validator func(s beam.Scope) // TimersEventTimeValidator produces a Validator that validates whether an OnTimer callback is invoked. // It passert.EqualsList expected timestamps (as millisecond int) plus an expected offset. // // makeImp is a func that produces an impulse (either beam.Impulse or beam.PeriodicImpulse). The // purpose of makeImp is to provide TimersEventTimeValidator either a bounded impulse or an // unbounded one. Beam, in general, uses this impulse to determine if a pipeline is "streaming" or // "batch". Test coverage for streaming or batch matters for executions on certain runners such as // Dataflow. func TimersEventTimeValidator(makeImp func(s beam.Scope) beam.PCollection) Validator { }

Validator is actually constructing the pipeline. It's not validating a pipeline. The validation has to be built in. It's not clear what benefit the additional indirection through a type (which is still exposed as a func(s beam.Scope) anyway serves here, and it's not going to be obvious a "Validator" is actually the same as expected by ptest.BuildAndRun, even though the compiler is happy with it.

I agree that the current TimersEventTime builder function isn't ideal. I'm going to unexport it, and simply move the full pipeline builds "internally", so it's not exposed.

I like how you re-worded the comment. It's more clear now. Feel free to resolve this conversation if you'd like.

sdks/go/test/integration/primitives/timers.go

sdks/go/test/integration/primitives/timers_test.go

lostluck · 2023-12-28T19:02:43Z

Great question! Fixing that is out of scope for this PR.

integration.go does not start runners. The Go SDK has never auto-started runners, largely due to that being an undocumented expectation that has never been fulfilled.

The lack of runner lead to deadline failure, and then retuning a nil Pipeline results. The test in particular doesn't fail out when there are nil PipelineResults though, so the metrics check then fails. This could be fixed by not returning a nil pipeline results in such cases in the default runner lib, or by making the test check if a nil was returned.

lostluck · 2023-12-28T19:34:47Z

Updated PTAL!

damondouglas

@lostluck LGTM

lostluck · 2023-12-28T22:03:20Z

Dataflow continues to pass, removing trigger files and merging.

Add eventTime Timer tests.

2ac0ad2

github-actions bot added go build labels Dec 19, 2023

lostluck added 2 commits December 19, 2023 21:23

Missing license.

fefb6a1

Filter test executions based on timer tests.

41dbc97

github-actions bot added the Next Action: Reviewers label Dec 20, 2023

Merge branch 'master' into timerIntegrationTests

da8b533

lostluck mentioned this pull request Dec 20, 2023

[Bug]: Go SDK uses PTransform UniqueName as key instead of the global PID for a transform. #29848

Closed

16 tasks

jrmccluskey reviewed Dec 27, 2023

View reviewed changes

sdks/go/test/integration/primitives/timers.go Outdated Show resolved Hide resolved

sdks/go/test/integration/primitives/timers.go Outdated Show resolved Hide resolved

lostluck changed the title ~~[#29772][Go SDK]Add EventTime Timer tests.~~ [#29772][Go SDK] Add EventTime Timer tests. Dec 28, 2023

Fix timer names

7b304de

damondouglas requested changes Dec 28, 2023

View reviewed changes

Update based on comments.

b8b5350

damondouglas self-requested a review December 28, 2023 20:28

damondouglas approved these changes Dec 28, 2023

View reviewed changes

Remove trigger files.

1fe6f00

lostluck merged commit 783c72a into apache:master Dec 28, 2023
7 checks passed

github-actions bot removed the build label Dec 28, 2023

lostluck deleted the timerIntegrationTests branch December 28, 2023 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#29772][Go SDK] Add EventTime Timer tests. #29829

[#29772][Go SDK] Add EventTime Timer tests. #29829

lostluck commented Dec 19, 2023

lostluck commented Dec 19, 2023

codecov bot commented Dec 19, 2023 •

edited

Loading

github-actions bot commented Dec 20, 2023

lostluck commented Dec 20, 2023

github-actions bot commented Dec 20, 2023

lostluck commented Dec 20, 2023

github-actions bot commented Dec 20, 2023

lostluck commented Dec 27, 2023

damondouglas left a comment

damondouglas Dec 28, 2023

lostluck Dec 28, 2023

damondouglas Dec 28, 2023

lostluck commented Dec 28, 2023

lostluck commented Dec 28, 2023

damondouglas left a comment

lostluck commented Dec 28, 2023

[#29772][Go SDK] Add EventTime Timer tests. #29829

[#29772][Go SDK] Add EventTime Timer tests. #29829

Conversation

lostluck commented Dec 19, 2023

GitHub Actions Tests Status (on master branch)

lostluck commented Dec 19, 2023

codecov bot commented Dec 19, 2023 • edited Loading

Codecov Report

github-actions bot commented Dec 20, 2023

lostluck commented Dec 20, 2023

github-actions bot commented Dec 20, 2023

lostluck commented Dec 20, 2023

github-actions bot commented Dec 20, 2023

lostluck commented Dec 27, 2023

damondouglas left a comment

Choose a reason for hiding this comment

damondouglas Dec 28, 2023

Choose a reason for hiding this comment

lostluck Dec 28, 2023

Choose a reason for hiding this comment

damondouglas Dec 28, 2023

Choose a reason for hiding this comment

lostluck commented Dec 28, 2023

lostluck commented Dec 28, 2023

damondouglas left a comment

Choose a reason for hiding this comment

lostluck commented Dec 28, 2023

codecov bot commented Dec 19, 2023 •

edited

Loading