You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2020/10/11 08:59:15 UTC
[GitHub] [beam] milantracy opened a new pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
milantracy opened a new pull request #13072:
URL: https://github.com/apache/beam/pull/13072
**Please** add a meaningful description for your change here
The PR follows how Java SDKs deal with the two options: `worker_region` and `worker_zone`
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) 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](https://www.apache.org/licenses/icla.pdf).
See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/#make-reviewers-job-easier).
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Dataflow | Flink | Samza | Spark | Twister2
--- | --- | --- | --- | --- | --- | ---
Go | [](https://ci-beam.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/) | ---
Java | [](https://ci-beam.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Twister2/lastCompletedBuild/)
Python | [](https://ci-beam.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python38/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/) | ---
XLang | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Direct/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | [](https://ci-beam.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/) | ---
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website | Whitespace | Typescript
--- | --- | --- | --- | --- | --- | ---
Non-portable | [](https://ci-beam.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/)<br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocker_Cron/lastCompletedBuild/) <br>[](https://ci-beam.apache.org/job/beam_PreCommit_PythonDocs_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Whitespace_Cron/lastCompletedBuild/) | [](https://ci-beam.apache.org/job/beam_PreCommit_Typescript_Cron/lastCompletedBuild/)
Portable | --- | [](https://ci-beam.apache.org/job/beam_PreCommit_Portable_Python_Cron/lastCompletedBuild/) | --- | --- | --- | ---
See [.test-infra/jenkins/README](https://github.com/apache/beam/blob/master/.test-infra/jenkins/README.md) for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
------------------------------------------------------------------------------------------------
[](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
[](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #13072:
URL: https://github.com/apache/beam/pull/13072#issuecomment-707266822
Run Go PostCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #13072:
URL: https://github.com/apache/beam/pull/13072#issuecomment-708026194
Be sure to mark the associated JIRA as resolved. Thanks again!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] milantracy commented on a change in pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
milantracy commented on a change in pull request #13072:
URL: https://github.com/apache/beam/pull/13072#discussion_r503570403
##########
File path: sdks/go/pkg/beam/runners/dataflow/dataflowlib/job.go
##########
@@ -262,3 +272,36 @@ func printOptions(opts *JobOptions, images []string) []*displayData {
}
return ret
}
+
+func validateWorkerSettings(opts *JobOptions) error {
Review comment:
added job_test.go
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] milantracy commented on a change in pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
milantracy commented on a change in pull request #13072:
URL: https://github.com/apache/beam/pull/13072#discussion_r503435751
##########
File path: sdks/go/gogradle.lock
##########
@@ -5,7 +5,7 @@ dependencies:
build:
- vcs: "git"
name: "cloud.google.com/go"
- commit: "4f6c921ec566a33844f4e7879b31cd8575a6982d"
+ commit: "03869a08dc16b35ad4968e92d34c5a2a2961b205"
Review comment:
The existing version of google.golang.org/api dependency doesn't have fields _worker_region_ and _worker_zone_. The change to google.golang.org/api version lead to other dependency changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] milantracy commented on pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
milantracy commented on pull request #13072:
URL: https://github.com/apache/beam/pull/13072#issuecomment-706673330
R: @lostluck @ibzib
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck commented on a change in pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
lostluck commented on a change in pull request #13072:
URL: https://github.com/apache/beam/pull/13072#discussion_r503452026
##########
File path: sdks/go/gogradle.lock
##########
@@ -5,7 +5,7 @@ dependencies:
build:
- vcs: "git"
name: "cloud.google.com/go"
- commit: "4f6c921ec566a33844f4e7879b31cd8575a6982d"
+ commit: "03869a08dc16b35ad4968e92d34c5a2a2961b205"
Review comment:
It's a pain in the butt to update/add new dependencies to the Go SDK, so it hasn't been kept up. This will change after we're on Go Modules for dependency versioning, which will happen after Schemas are finished implementation in the SDK. (I'm working on it). That would also allow users to use the same deps as the project uses for testing. Right now, this list is functionally for Jenkin's Use Only.
While your proposal to split the changes is generally a good policy, I disagree that it's necessary to split the changes in this instance. There's not nearly as much surface area vs the Python and Java SDKs, and there are no real runners implemented in Go. If the PostCommit test passes, there's little else we do that would detect issues with GRPC or similar.
Also, internal testing at Google is likely using much more up to date versions of all these packages so this just brings things closer to that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #13072:
URL: https://github.com/apache/beam/pull/13072#discussion_r503439527
##########
File path: sdks/go/gogradle.lock
##########
@@ -5,7 +5,7 @@ dependencies:
build:
- vcs: "git"
name: "cloud.google.com/go"
- commit: "4f6c921ec566a33844f4e7879b31cd8575a6982d"
+ commit: "03869a08dc16b35ad4968e92d34c5a2a2961b205"
Review comment:
> The existing version of google.golang.org/api dependency doesn't have fields worker_region and worker_zone.
I see. I was not expecting that because worker_region and worker_zone were added a long time ago. The old Google API library is from Feb 7 2018.
Anyway, we should split the dependency changes and feature implementation into separate commits.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #13072:
URL: https://github.com/apache/beam/pull/13072#discussion_r503489873
##########
File path: sdks/go/gogradle.lock
##########
@@ -5,7 +5,7 @@ dependencies:
build:
- vcs: "git"
name: "cloud.google.com/go"
- commit: "4f6c921ec566a33844f4e7879b31cd8575a6982d"
+ commit: "03869a08dc16b35ad4968e92d34c5a2a2961b205"
Review comment:
> While your proposal to split the changes is generally a good policy, I disagree that it's necessary to split the changes in this instance.
Not necessary, just a matter of personal preference (they can just be separate commits, it's fine if they're merged as part of the same PR).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] milantracy commented on a change in pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
milantracy commented on a change in pull request #13072:
URL: https://github.com/apache/beam/pull/13072#discussion_r503570323
##########
File path: sdks/go/gogradle.lock
##########
@@ -5,7 +5,7 @@ dependencies:
build:
- vcs: "git"
name: "cloud.google.com/go"
- commit: "4f6c921ec566a33844f4e7879b31cd8575a6982d"
+ commit: "03869a08dc16b35ad4968e92d34c5a2a2961b205"
Review comment:
thanks both for the elaboration.
i will merge changes to the dependencies in one PR since it only impacts jenkins
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] lostluck merged pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
lostluck merged pull request #13072:
URL: https://github.com/apache/beam/pull/13072
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] ibzib commented on a change in pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
ibzib commented on a change in pull request #13072:
URL: https://github.com/apache/beam/pull/13072#discussion_r503424490
##########
File path: sdks/go/gogradle.lock
##########
@@ -5,7 +5,7 @@ dependencies:
build:
- vcs: "git"
name: "cloud.google.com/go"
- commit: "4f6c921ec566a33844f4e7879b31cd8575a6982d"
+ commit: "03869a08dc16b35ad4968e92d34c5a2a2961b205"
Review comment:
This PR shouldn't require any changes to dependencies.
##########
File path: sdks/go/pkg/beam/runners/dataflow/dataflowlib/job.go
##########
@@ -262,3 +272,36 @@ func printOptions(opts *JobOptions, images []string) []*displayData {
}
return ret
}
+
+func validateWorkerSettings(opts *JobOptions) error {
Review comment:
Add tests for this function.
##########
File path: sdks/go/pkg/beam/runners/dataflow/dataflowlib/job.go
##########
@@ -262,3 +272,36 @@ func printOptions(opts *JobOptions, images []string) []*displayData {
}
return ret
}
+
+func validateWorkerSettings(opts *JobOptions) error {
+ if opts.Zone != "" && opts.WorkerRegion != "" {
+ return errors.New("cannot use option zone with workerRegion; prefer either workerZone or workerRegion")
+ }
+ if opts.Zone != "" && opts.WorkerZone != "" {
+ return errors.New("cannot use option zone with workerZone; prefer workerZone")
+ }
+ if opts.WorkerZone != "" && opts.WorkerRegion != "" {
+ return errors.New("workerRegion and workerZone options are mutually exclusive")
+ }
+
+ hasExperimentWorkerRegion := false
+ for _, experiment := range opts.Experiments {
+ if strings.HasPrefix(experiment, "worker_region") {
+ hasExperimentWorkerRegion = true
+ break
+ }
+ }
+
+ if hasExperimentWorkerRegion && opts.WorkerRegion != "" {
+ return errors.New("experiment worker_region and option workerRegion are mutually exclusive")
+ }
+ if hasExperimentWorkerRegion && opts.WorkerZone != "" {
+ return errors.New("experiment worker_region and option workerZone are mutually exclusive")
+ }
+
+ if opts.Zone != "" {
Review comment:
Print a warning that zone is deprecated and worker_zone should be used instead.
##########
File path: sdks/go/pkg/beam/runners/dataflow/dataflowlib/job.go
##########
@@ -262,3 +272,36 @@ func printOptions(opts *JobOptions, images []string) []*displayData {
}
return ret
}
+
+func validateWorkerSettings(opts *JobOptions) error {
+ if opts.Zone != "" && opts.WorkerRegion != "" {
+ return errors.New("cannot use option zone with workerRegion; prefer either workerZone or workerRegion")
+ }
+ if opts.Zone != "" && opts.WorkerZone != "" {
+ return errors.New("cannot use option zone with workerZone; prefer workerZone")
+ }
+ if opts.WorkerZone != "" && opts.WorkerRegion != "" {
+ return errors.New("workerRegion and workerZone options are mutually exclusive")
+ }
+
+ hasExperimentWorkerRegion := false
+ for _, experiment := range opts.Experiments {
+ if strings.HasPrefix(experiment, "worker_region") {
+ hasExperimentWorkerRegion = true
+ break
+ }
+ }
+
+ if hasExperimentWorkerRegion && opts.WorkerRegion != "" {
Review comment:
There also needs to be a check for `hasExperimentWorkerRegion && opts.Zone != ""`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] milantracy commented on pull request #13072: [BEAM-8251] Add worker_region and worker_zone options
Posted by GitBox <gi...@apache.org>.
milantracy commented on pull request #13072:
URL: https://github.com/apache/beam/pull/13072#issuecomment-707387342
Run Go PostCommit
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org