[
https://issues.apache.org/jira/browse/BEAM-14505?focusedWorklogId=774797&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-774797
]
ASF GitHub Bot logged work on BEAM-14505:
-----------------------------------------
Author: ASF GitHub Bot
Created on: 25/May/22 19:57
Start Date: 25/May/22 19:57
Worklog Time Spent: 10m
Work Description: damccorm commented on code in PR #17747:
URL: https://github.com/apache/beam/pull/17747#discussion_r882061858
##########
sdks/go/pkg/beam/runners/dataflow/dataflowlib/job.go:
##########
@@ -239,6 +252,13 @@ func Translate(ctx context.Context, p *pipepb.Pipeline,
opts *JobOptions, worker
// Submit submits a prepared job to Cloud Dataflow.
func Submit(ctx context.Context, client *df.Service, project, region string,
job *df.Job) (*df.Job, error) {
+ if job.ReplaceJobId != "" {
Review Comment:
A cleaner way to do this would be to add replaceJobId as a boolean parameter
here. We already have the JobOptions in the only thing calling this -
https://github.com/jrmccluskey/beam/blob/ba8ffc295e49985ef93215808d4cd748845f038b/sdks/go/pkg/beam/runners/dataflow/dataflowlib/execute.go#L102
##########
sdks/go/pkg/beam/runners/dataflow/dataflow_test.go:
##########
@@ -197,3 +197,50 @@ func TestGetJobOptions_DockerNoImage(t *testing.T) {
t.Fatalf("getContainerImage() = %q, want %q", got, want)
}
}
+
+func TestGetJobOptions_MappingNoUpdate(t *testing.T) {
Review Comment:
Could you update (here and similarly in the next test) to
`TestGetJobOptions_TransformMappingNoUpdate` - if I didn't have the context of
this pr I wouldn't know what was being tested.
##########
sdks/go/pkg/beam/runners/dataflow/dataflowlib/job.go:
##########
@@ -83,6 +87,8 @@ type JobOptions struct {
TeardownPolicy string
}
+const replaceIdPlaceholder = "toBeReplaced"
Review Comment:
Nit - making this a constant vs inlining doesn't get us anything since we're
not reusing it (and its not meaningful)
##########
sdks/go/pkg/beam/runners/dataflow/dataflow_test.go:
##########
@@ -197,3 +197,50 @@ func TestGetJobOptions_DockerNoImage(t *testing.T) {
t.Fatalf("getContainerImage() = %q, want %q", got, want)
}
}
+
+func TestGetJobOptions_MappingNoUpdate(t *testing.T) {
Review Comment:
Also, its probably worth adding one more test for the happy path
Issue Time Tracking
-------------------
Worklog Id: (was: 774797)
Time Spent: 1h (was: 50m)
> Add Dataflow streaming pipeline update support to the Go SDK
> ------------------------------------------------------------
>
> Key: BEAM-14505
> URL: https://issues.apache.org/jira/browse/BEAM-14505
> Project: Beam
> Issue Type: Improvement
> Components: sdk-go
> Reporter: Jack McCluskey
> Assignee: Jack McCluskey
> Priority: P2
> Time Spent: 1h
> Remaining Estimate: 0h
>
> Add support for updating streaming pipelines on Dataflow, allowing Go SDK
> users to follow the same processes as users of the Java and Python SDKs. See
> [https://cloud.google.com/dataflow/docs/guides/updating-a-pipeline] for
> information on how the user experience works for those SDKs.
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
