[BEAM-11574] Enable cross-language integration tests on Dataflow #14397

youngoli · 2021-03-31T22:53:28Z

Contains a variety of small changes needed to enable cross-language tests to work on Dataflow. Here's a list of changes:

Allow running pipelines with portable job submission (required for x-lang transforms).
Allow submitting Dataflow jobs with multiple environments.
Add SdkHarnessContainerImageOverrides flag to allow specifying overrides for multiple environments. This implementation works by calling the flag multiple times, once for each override.
Update the ValidatesRunner script to use the above functionality (portable submission and overrides) as well as uploading a Java SDK container for cross-language in addition to a Go SDK container.
Don't namespace environments in external transform expansion. This avoids bugs that occurred because an environment with an empty value was present in the final pipeline.
Skip fake impulse generation in external transform expansion. Something must have changed on the expansion service end, because this is no longer necessary, and was causing problems because the fake impulse wasn't getting properly removed.

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

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
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.

Post-Commit Tests Status (on master branch)

Lang	ULR	Dataflow	Samza	Twister2
Go	---	---	---	---
Java
Python	---		---	---
XLang	---		---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website	Whitespace	Typescript
Non-portable
Portable	---		---	---	---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

GitHub Actions Tests Status (on master branch)

See CI.md for more information about GitHub Actions CI.

youngoli · 2021-03-31T22:53:48Z

Run Go PostCommit

Contains a variety of small changes needed to enable cross-language tests to work on Dataflow. Here's a list of changes: 1. Allow running pipelines with portable job submission (required for x-lang transforms). 2. Allow submitting Dataflow jobs with multiple environments. 3. Add SdkHarnessContainerImageOverrides flag to allow specifying overrides for multiple environments. This implementation works by calling the flag multiple times, once for each override. 4. Update the ValidatesRunner script to use the above functionality (portable submission and overrides) as well as uploading a Java SDK container for cross-language in addition to a Go SDK container. 5. Don't namespace environments in external transform expansion. This avoids bugs that occurred because an environment with an empty value was present in the final pipeline. 6. Skip fake impulse generation in external transform expansion. Something must have changed on the expansion service end, because this is no longer necessary, and was causing problems because the fake impulse wasn't getting properly removed.

youngoli · 2021-03-31T23:12:29Z

Run Go PostCommit

youngoli · 2021-03-31T23:21:26Z

Run GoPortable PreCommit

youngoli · 2021-04-01T00:32:19Z

R: @lostluck

lostluck

LGTM after the changes. Mostly typos and smoke checks.

lostluck · 2021-04-02T18:18:16Z

sdks/go/pkg/beam/core/runtime/pipelinex/replace.go

+		}
+		oldImg := payload.GetContainerImage()
+		for pattern, replacement := range patterns {
+			re, err := regexp.Compile(pattern)


No action required, just a call out to be wary of nested for loops and repeated work.

I think my main concern here is that we're compiling the patterns multiple times (each once per environment at most), which is probably not going to be terrible since this is at construction time, and both patterns and environments are going to be limited.

Good point. I went ahead and fixed it anyway, and yeah I'll keep that in mind.

lostluck · 2021-04-02T18:21:10Z

sdks/go/pkg/beam/core/runtime/pipelinex/replace.go

+// ApplySdkImageOverrides takes a pipeline and a map of patterns to overrides,
+// and proceeds to replace matching ContainerImages in any Environments
+// present in the pipeline.
+func ApplySdkImageOverrides(p *pipepb.Pipeline, patterns map[string]string) error {


Consider documenting the expectation that any given environment is expected to match only a single pattern for replacement, and that if multiple patterns would match, it's arbitrary which will be applied (due to map iteration ordering being random.)

There's no good way to handle such conflict cases for multple matches. I suspect the most we can do is say it's undefined in the flag itself, as well as the commentary change in the previous paragraph.

Done. Also adding this to the flag description.

lostluck · 2021-04-02T18:23:21Z

sdks/go/pkg/beam/core/runtime/xlangx/namespace.go

-	if t.EnvironmentId != "" {
-		t.EnvironmentId = addEnvironmentID(c, idMap, t.EnvironmentId, newID)
-	}
+	// TODO: Currently environments are not namespaced. This works under the


Consider including the JIRA tag for history context, or instead of TODO (which implies work that should likely be done) use Note: or avoid a prefix entirely, since all comments are notes to our future selves.

As for the content, it seems probable that we'd make sure the Go ExpansionService that handles these calls would do the namespacing on the response itself. That way it can build the pipeline graph normally, and we simply run the replacement there, where we are already certain that it's a foreign component, rather than the primary pipeline.

Changed it to a "note". That makes more sense than making a JIRA for the vague possibility of multiple Go environments.

lostluck · 2021-04-02T18:24:39Z

sdks/go/pkg/beam/options/jobopts/options.go

+		"sdk_harness_container_image_override",
+		"Overrides for SDK harness container images. Could be for the "+
+			"local SDK or for a remote SDK that pipeline has to support due "+
+			"to a cross-language transform. Each entry consist of two values "+


Suggested change

"to a cross-language transform. Each entry consist of two values "+

"to a cross-language transform. Each entry consists of two values "+

lostluck · 2021-04-02T18:28:07Z

sdks/go/pkg/beam/options/jobopts/options.go

+			"separated by a comma where first value gives a regex to "+
+			"identify the container image to override and the second value "+
+			"gives the replacement container image. Multiple entries can be "+
+			"specified by using this flag multiple times.")


Just confirming, this part matches the semantics of the equivalent flag in the other SDKs?

I haven't been able to find the documentation for the Java version of this flag (despite being relatively sure it exists, based on this line), but the Python version works identically, it requires you to use the flag once per entry.

lostluck · 2021-04-02T18:30:05Z

sdks/go/pkg/beam/runners/dataflow/dataflow.go

+	if err != nil {
+		return nil, errors.WithContext(err, "generating model pipeline")
+	}
+	err = pipelinex.ApplySdkImageOverrides(model, jobopts.GetSdkImageOverrides())
 	if err != nil {
 		return nil, errors.WithContext(err, "generating model pipeline")


Suggested change

return nil, errors.WithContext(err, "generating model pipeline")

return nil, errors.WithContext(err, "applying container image overrides")

Done, also updated the one a few lines above.

youngoli · 2021-04-05T22:07:47Z

Run Go Flink ValidatesRunner

lostluck · 2021-04-06T15:39:52Z

There's light discussion around the format of the override flag happening, but that's not a reason to block this change from getting in.

youngoli force-pushed the xlang2 branch from 5bd329e to 31be314 Compare March 31, 2021 23:11

lostluck approved these changes Apr 2, 2021

View reviewed changes

youngoli mentioned this pull request Apr 3, 2021

[BEAM-12060] Fix failing Go Postcommits, jenkins support for Gradle tasks. #14407

Merged

4 tasks

Fixup and adding missing test.

5f632cf

lostluck mentioned this pull request Apr 5, 2021

[BEAM-12060] Remove overwriting jenkins property. #14432

Merged

4 tasks

youngoli merged commit 9ec4582 into apache:master Apr 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-11574] Enable cross-language integration tests on Dataflow #14397

[BEAM-11574] Enable cross-language integration tests on Dataflow #14397

youngoli commented Mar 31, 2021

youngoli commented Mar 31, 2021

youngoli commented Mar 31, 2021

youngoli commented Mar 31, 2021

youngoli commented Apr 1, 2021

lostluck left a comment

lostluck Apr 2, 2021

youngoli Apr 3, 2021

lostluck Apr 2, 2021

youngoli Apr 3, 2021

lostluck Apr 2, 2021

youngoli Apr 3, 2021

lostluck Apr 2, 2021

youngoli Apr 3, 2021

lostluck Apr 2, 2021

youngoli Apr 3, 2021

lostluck Apr 2, 2021

youngoli Apr 3, 2021

youngoli commented Apr 5, 2021

lostluck commented Apr 6, 2021

	"to a cross-language transform. Each entry consist of two values "+
	"to a cross-language transform. Each entry consists of two values "+

	return nil, errors.WithContext(err, "generating model pipeline")
	return nil, errors.WithContext(err, "applying container image overrides")

[BEAM-11574] Enable cross-language integration tests on Dataflow #14397

[BEAM-11574] Enable cross-language integration tests on Dataflow #14397

Conversation

youngoli commented Mar 31, 2021

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

GitHub Actions Tests Status (on master branch)

youngoli commented Mar 31, 2021

youngoli commented Mar 31, 2021

youngoli commented Mar 31, 2021

youngoli commented Apr 1, 2021

lostluck left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngoli commented Apr 5, 2021

lostluck commented Apr 6, 2021