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 2022/06/16 00:11:06 UTC
[GitHub] [beam-starter-go] davidcavazos opened a new pull request, #1: Initial commit
davidcavazos opened a new pull request, #1:
URL: https://github.com/apache/beam-starter-go/pull/1
Initial commit on Go starter project.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] lostluck commented on pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#issuecomment-1163754205
Merged!
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] lostluck commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r898622963
##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+type Result = struct {
+ Pipeline *beam.Pipeline
+ Scope beam.Scope
+ PCollection beam.PCollection
+}
+
+func my_pipeline(input_text string) Result {
+ beam.Init()
Review Comment:
This should be called in `main`, right after flag parsing.
##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+type Result = struct {
+ Pipeline *beam.Pipeline
+ Scope beam.Scope
+ PCollection beam.PCollection
+}
+
+func my_pipeline(input_text string) Result {
+ beam.Init()
+
+ pipeline, scope := beam.NewPipelineWithRoot()
+
+ elements := beam.Create(scope, "Hello", "World!", input_text)
+ elements = beam.ParDo(scope, func(elem string, emit func(string)) { fmt.Println(elem); emit(elem) }, elements)
Review Comment:
While an anonymous function *can* work in Beam Go, it will cause more trouble than it's worth. Move it to a standard definition, and use an init block to register the function.
Otherwise, folks moving beyond the direct runner will have a bad time.
```
func printAndEmit(elem string, emit(string)) {
fmt.Println(elem)
emit(elem)
}
func init() {
// DoFns should be registered with Beam.
beam.RegisterFunction(printAndEmit)
}
```
##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+type Result = struct {
Review Comment:
Secondly, while one *can* define a type for this, one shouldn't. Go would just use multiple returns.
In this case, I would recommend calling `pipeline, scope := beam.NewPipelineWithRoot()` in `main`, and passing in the scope variable. Then we simply return the PCollection.
##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+type Result = struct {
Review Comment:
Type definitions don't use an `=`. Using an equal defines a type alias, which isn't what we want here.
```suggestion
type Result struct {
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] davidcavazos commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
davidcavazos commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r899424744
##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+type Result = struct {
+ Pipeline *beam.Pipeline
+ Scope beam.Scope
+ PCollection beam.PCollection
+}
+
+func my_pipeline(input_text string) Result {
+ beam.Init()
Review Comment:
Sounds good, moved.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] davidcavazos commented on pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
davidcavazos commented on PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#issuecomment-1163657201
Hi @lostluck, I can't merge it. It would be great if you helped us merge it :)
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] lostluck commented on pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#issuecomment-1163614776
Since this is in the apache org, I can merge this if you can't. Please let me know, and it'll be done.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] davidcavazos commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
davidcavazos commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r900499324
##########
main.go:
##########
@@ -14,31 +14,32 @@ var (
input_text = flag.String("input-text", "Default input text", "Input text to print.")
)
-type Result = struct {
- Pipeline *beam.Pipeline
- Scope beam.Scope
- PCollection beam.PCollection
+func init() {
+ // DoFns should be registered with Beam.
+ beam.RegisterFunction(printAndEmit)
}
-func my_pipeline(input_text string) Result {
- beam.Init()
-
- pipeline, scope := beam.NewPipelineWithRoot()
+func printAndEmit(element string, emit func(string)) {
+ fmt.Println(element)
Review Comment:
Sounds good, I created an extra function to show how to do a "simple" function and a function that takes a Context and emits values.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] davidcavazos commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
davidcavazos commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r900499652
##########
.github/workflows/test.yaml:
##########
@@ -0,0 +1,22 @@
+# Copyright 2022 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+# https://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+# <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your
+# option. This file may not be copied, modified, or distributed
+# except according to those terms.
+
+name: Test
+
+on: [push, pull_request]
+
+jobs:
+ tests:
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v3
+ - uses: actions/setup-go@v3
+ with:
+ go-version: '>=1.18.0'
Review Comment:
Good catch, thanks!
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] davidcavazos commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
davidcavazos commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r899422619
##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+type Result = struct {
Review Comment:
Got it, that's a great suggestion!
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] pcoet commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
pcoet commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r899658199
##########
CONTRIBUTING.md:
##########
@@ -0,0 +1,69 @@
+# Contributing
+
+🎉🎊 Thanks for taking the time to contribute! 🎉🎊
+
+There are many ways to contribute, here are some.
+
+## Filing an issue
+
+If there's any issue you encounter or anything that needs to be fixed, feel free to [create a GitHub issue](https://github.com/apache/beam-starter-go/issues).
+
+## Contributing to this starter project
+
+If this is your first time contributing to a GitHub repo,
+we recommmend going through the
+[GitHub quickstart](https://docs.github.com/en/get-started/quickstart/hello-world).
+
+It's a good idea to discuss your plans with the Beam community through the dev@beam.apache.org mailing list before doing any changes.
+
+Here's a small overview of the process.
+
+1. [Fork the repo](https://docs.github.com/en/get-started/quickstart/fork-a-repo).
+
+1. Clone the repo.
+
+ ```sh
+ export GITHUB_USERNAME="my-github-username"
+
+ git clone git@github.com:$GITHUB_USERNAME/beam-starter-go.git
+ ```
+
+1. Set the [upstream remote branch](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/configuring-a-remote-for-a-fork).
+
+ ```sh
+ cd beam-starter-go
+ git remote add upstream git@github.com:apache/beam-starter-go.git
+ ```
+
+1. Create and change to a new branch.
+
+ ```sh
+ git checkout -B my-branch-name
+ ```
+
+1. Modify the code! 😱
+
+1. Run the tests. For steps on how to run them see the [`README.md`](README.md).
+
+1. Commit and push your changes to your branch in `origin`.
+
+ ```sh
+ git commit -m "one line description of your changes"
+ git push
+ ```
+
+1. [Create a Pull Request](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request).
+
+1. Add reviewers, and [address review comments](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests).
+
+1. Once it's approved, we can merge the Pull Request.
+
+For more information about proposing changes to a GitHub repository, see the
+[Propose changes](https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/about-branches)
+page in the GitHub docs.
+
+## Contributing to Apache Beam
+
+For information on how to contribute to
+[Apache Beam](https://github.com/apache/beam), see to the
Review Comment:
"see to the" -> "see the"
##########
CONTRIBUTING.md:
##########
@@ -0,0 +1,69 @@
+# Contributing
+
+🎉🎊 Thanks for taking the time to contribute! 🎉🎊
+
+There are many ways to contribute, here are some.
Review Comment:
"contribute, here" -> "contribute. Here"
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] lostluck commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r900425256
##########
.github/workflows/test.yaml:
##########
@@ -0,0 +1,22 @@
+# Copyright 2022 Google LLC
+#
+# Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+# https://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+# <LICENSE-MIT or https://opensource.org/licenses/MIT>, at your
+# option. This file may not be copied, modified, or distributed
+# except according to those terms.
+
+name: Test
+
+on: [push, pull_request]
+
+jobs:
+ tests:
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v3
+ - uses: actions/setup-go@v3
+ with:
+ go-version: '>=1.18.0'
Review Comment:
Reduce maintenance for us and users, and have the version pulled from the go.mod file: https://github.com/actions/setup-go#getting-go-version-from-the-gomod-file
So for this it's probably `go-version-file: 'go.mod'`
##########
main.go:
##########
@@ -0,0 +1,45 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+func init() {
+ // DoFns should be registered with Beam.
+ beam.RegisterFunction(printAndEmit)
+}
+
+func printAndEmit(element string, emit func(string)) {
+ fmt.Println(element)
+ emit(element)
+}
+
+func my_pipeline(scope beam.Scope, input_text string) beam.PCollection {
+ elements := beam.Create(scope, "Hello", "World!", input_text)
+ elements = beam.ParDo(scope, printAndEmit, elements)
+ return elements
+}
+
+func main() {
+ flag.Parse()
+ beam.Init()
+
+ ctx := context.Background()
+ pipeline, scope := beam.NewPipelineWithRoot()
+ _ = my_pipeline(scope, *input_text)
Review Comment:
Since we aren't using the return, we can simply avoid mentioning it here.
```suggestion
my_pipeline(scope, *input_text)
```
##########
main.go:
##########
@@ -14,31 +14,32 @@ var (
input_text = flag.String("input-text", "Default input text", "Input text to print.")
)
-type Result = struct {
- Pipeline *beam.Pipeline
- Scope beam.Scope
- PCollection beam.PCollection
+func init() {
+ // DoFns should be registered with Beam.
+ beam.RegisterFunction(printAndEmit)
}
-func my_pipeline(input_text string) Result {
- beam.Init()
-
- pipeline, scope := beam.NewPipelineWithRoot()
+func printAndEmit(element string, emit func(string)) {
+ fmt.Println(element)
Review Comment:
import the Beam [log package](https://pkg.go.dev/github.com/apache/beam/sdks/v2/go/pkg/beam/log), and use log.Infoln instead of fmt.Println. This allows the print to be visible from distributed / portable runners. fmt.Println will simply have the output lost to the worker container.
##########
main.go:
##########
@@ -0,0 +1,45 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+func init() {
+ // DoFns should be registered with Beam.
+ beam.RegisterFunction(printAndEmit)
+}
+
+func printAndEmit(element string, emit func(string)) {
+ fmt.Println(element)
+ emit(element)
+}
+
+func my_pipeline(scope beam.Scope, input_text string) beam.PCollection {
Review Comment:
```suggestion
func myPipeline(scope beam.Scope, input_text string) beam.PCollection {
```
Go style has methods/functions use camelcase rather than snake case (snake case is for flags).
##########
main.go:
##########
@@ -14,31 +14,32 @@ var (
input_text = flag.String("input-text", "Default input text", "Input text to print.")
)
-type Result = struct {
- Pipeline *beam.Pipeline
- Scope beam.Scope
- PCollection beam.PCollection
+func init() {
+ // DoFns should be registered with Beam.
+ beam.RegisterFunction(printAndEmit)
}
-func my_pipeline(input_text string) Result {
- beam.Init()
-
- pipeline, scope := beam.NewPipelineWithRoot()
+func printAndEmit(element string, emit func(string)) {
+ fmt.Println(element)
+ emit(element)
+}
+func my_pipeline(scope beam.Scope, input_text string) beam.PCollection {
elements := beam.Create(scope, "Hello", "World!", input_text)
- elements = beam.ParDo(scope, func(elem string, emit func(string)) { fmt.Println(elem); emit(elem) }, elements)
-
- return Result{pipeline, scope, elements}
+ elements = beam.ParDo(scope, printAndEmit, elements)
+ return elements
Review Comment:
Style wise, this is a code smell. Simpler to simply return without the variable.
```suggestion
return beam.ParDo(scope, printAndEmit, elements)
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] lostluck merged pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
lostluck merged PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] brucearctor commented on pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
brucearctor commented on PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#issuecomment-1164992775
Wohooo! Congrats!
On Wed, Jun 22, 2022 at 4:20 PM Robert Burke ***@***.***>
wrote:
> Merged!
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/beam-starter-go/pull/1#issuecomment-1163754205>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/ABGMTJCJU4QWTY57ZEJQFLTVQONUVANCNFSM5Y5BA4PA>
> .
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam-starter-go] davidcavazos commented on a diff in pull request #1: Initial commit
Posted by GitBox <gi...@apache.org>.
davidcavazos commented on code in PR #1:
URL: https://github.com/apache/beam-starter-go/pull/1#discussion_r899425295
##########
main.go:
##########
@@ -0,0 +1,44 @@
+package main
+
+import (
+ "context"
+ "flag"
+ "fmt"
+ "log"
+
+ "github.com/apache/beam/sdks/v2/go/pkg/beam"
+ "github.com/apache/beam/sdks/v2/go/pkg/beam/x/beamx"
+)
+
+var (
+ input_text = flag.String("input-text", "Default input text", "Input text to print.")
+)
+
+type Result = struct {
+ Pipeline *beam.Pipeline
+ Scope beam.Scope
+ PCollection beam.PCollection
+}
+
+func my_pipeline(input_text string) Result {
+ beam.Init()
+
+ pipeline, scope := beam.NewPipelineWithRoot()
+
+ elements := beam.Create(scope, "Hello", "World!", input_text)
+ elements = beam.ParDo(scope, func(elem string, emit func(string)) { fmt.Println(elem); emit(elem) }, elements)
Review Comment:
That's good to know! Done.
--
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.
To unsubscribe, e-mail: github-unsubscribe@beam.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org