[BEAM-9339] Declare capabilities for Go SDK. #10911
Conversation
|
R: @lostluck |
There was a problem hiding this comment.
We may want to consider just moving the code in universal to graphx/translate.go since that's more central and defines the "default" go environment, so it's also in the pipeline proto for non universal runners (Dataflow).
However, this sets the bar so we know if that change accidentally breaks it later. Or I'm missing something since this is being reviewed on my phone .
| capabilities = append(capabilities, graphx.KnownStandardCoders()...) | ||
| return capabilities | ||
| } | ||
|
|
There was a problem hiding this comment.
You can return straight from the append and save a line.
There was a problem hiding this comment.
Done. I wasn't sure what the convention was--the semantics of append (may modify the first argument or return a new value) seemed odd to me.
There was a problem hiding this comment.
Fair enough.
Technically append always returns a new "slice" but the backing array for the data may be reused. It's possible to have aliasing problems if done poorly, but that's not a risk here.
It mostly comes down to the difference between the used length, and the capacity of the slice. If there's enough capacity, it will simply mutate the backing slice.
To avoid the aliasing, as a rule, one should avoid using an slice one didn't allocate as the base for append.
|
I'll defer further refactoring, but could you clarify your comment about Dataflow? Are we not creating/setting an environment at all in that case? |
|
Dataflow creates it's own environment and doesn't use the universal runner. |
|
OK, we do need this for dataflow. I refactored the code to be shared in translate.go. PTAL. |
| @@ -50,6 +50,22 @@ const ( | |||
| urnCoGBKList = "beam:go:coder:cogbklist:v1" // CoGBK representation. Not a coder. | |||
| ) | |||
|
|
|||
| func KnownStandardCoders() []string { | |||
There was a problem hiding this comment.
Since this lives in the same package as the caller, we can make the function un-exported (lowercase k for knownStandardCoders() instead).
It's reasonably to have this function live here, due to locality with the coder URNs
| @@ -26,6 +27,7 @@ import ( | |||
| "github.com/apache/beam/sdks/go/pkg/beam/core/util/protox" | |||
| "github.com/apache/beam/sdks/go/pkg/beam/internal/errors" | |||
| pb "github.com/apache/beam/sdks/go/pkg/beam/model/pipeline_v1" | |||
| "github.com/apache/beam/sdks/go/pkg/beam/options/jobopts" | |||
There was a problem hiding this comment.
I don't like that we're importing this package here. jobopts creates flags, which could be unexpected or unnecessary for certain runners.
Could we instead just pass the environment urn in as a string? (the only thing we actually use from the package).
robertwb
force-pushed
the
go-capabilities
branch
from
14594c4
to
5f46b67
Compare
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.