[Beam-9679] Add Core Transforms section / ParDo lesson to the Go SDK katas #11564

damondouglas · 2020-04-29T04:28:40Z

This pull request adds a section related to Core Transforms with one Map transforms lesson to the Go SDK katas. It patterns after the existing Java katas:

ParDo
ParDo OneToMany
ParDo struct

I would like to request @henryken and @lostluck to review this pull request. Thank you for allowing me to be part of this project.

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	SDK	Apex	Dataflow	Gearpump	Samza
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

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

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

lostluck · 2020-04-29T19:50:52Z

Run RAT Precommit

lostluck · 2020-04-29T19:51:00Z

Run Go Precommit

henryken

Thanks for the addition of the new lessons. I've added some comments.

learning/katas/go/Core Transforms/Map/ParDo/test/task_test.go

learning/katas/go/Core Transforms/Map/ParDo/cmd/main.go

learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go

learning/katas/go/Core Transforms/Map/MapElements/task.md

learning/katas/go/Core Transforms/Map/MapElements/pkg/task/task.go

learning/katas/go/Core Transforms/Map/FlatMapElements/task.md

damondouglas · 2020-05-01T02:49:00Z

Run RAT Precommit

damondouglas · 2020-05-01T02:49:11Z

Run Go Precommit

henryken

Thanks Damon for incorporating the changes. Just a few additional minor comments.

learning/katas/go/Core Transforms/Map/lesson-info.yaml

learning/katas/go/Core Transforms/Map/ParDo/pkg/task/task.go

learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go

learning/katas/go/Core Transforms/Map/ParDo struct/task.md

learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go

damondouglas · 2020-05-03T19:16:35Z

Run RAT Precommit

henryken

The learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go seems to have an issue now.
I suspect the directory needs to be renamed accordingly.

learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go

learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go

damondouglas · 2020-05-04T14:18:09Z

Thank you @henryken for your patience. I found the course preview function in IntelliJ Edu inconsistent between GoLand and IntelliJ. In my regular testing using GoLand, I load by first closing the project and reopening via navigation to the original folder. If I open the project through any other means in GoLand, when activating the course preview, it seems to pull from a cached source. I haven't completely figured out the issue with GoLand. IntelliJ seems to work as expected.

I deleted the entire ParDo struct and recreated it to ParDo Struct as well as removed my entire src and pkg folder in my $GOHOME. I tested the course preview in both GoLand and IntelliJ and it masks the code using the placeholders where expected.

lostluck · 2020-05-04T18:04:45Z

Retest this please

lostluck · 2020-05-04T18:05:34Z

(sadly, only comitters can trigger the tests even after the first time. Your commands were correct)
I'm doing this review now. Thank you for your patience!

damondouglas · 2020-05-04T18:12:58Z

Retest this please

Is this addressed to me? Would you like me to retest something?

That's "addressed" to the jenkins bot to retest the PR via whatever automated selection of tests it requires. Since the bot is particular, I needed to separate it from my other comment so it would work.

lostluck

My only question might be better answred by @henryken : Are the Spaces in the directory names going to cause problems with katas? Are they intended to be like that vs with underscores instead of spaces?

Otherwise, just a few small things, and I can merge this.

learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md

lostluck · 2020-05-04T18:10:57Z

learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md

+
+<div class="hint">
+  Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo">
+  ParDo</a>


Consider using the qualifed import name, like they'd see in their Go code (eg. beam.ParDo) , rather than just the single method. It would look odd to other languages, but the generally explicit package/provenance of identifiers is a hallmark of Go.

@lostluck Do you mean this? If so, I could check across the various tasks of the existing katas to make sure its consistent.

<div class="hint"> Use <a href="https://godoc.org/github.com/apache/beam/sdks/go/pkg/beam#ParDo"> beam.ParDo</a>

Rather, I do. But I can merge this in as is and the change can happen in another PR or it can happen this PR. Whichever you prefer.

Sorry for the delay in getting back to this.

I created BEAM-9928 so this is not forgotten. My vote is to merge the request.

lostluck · 2020-05-04T18:17:06Z

learning/katas/go/Core Transforms/Map/ParDo/task.md

+performs some processing function (your user code) on that element, and emits zero, one, or multiple elements to an 
+output PCollection.
+
+**Kata:** Please write a simple ParDo that maps the input element by multiplying it by 10.


A possible adjacent task/step is converting a func DoFn into a Structural DoFn. Yes, this is relatively simple, but from a learning standpoint, it makes the distinction pretty clear, while not asking users too much else that might conflate with it. (eg. Getting the wrong idea that funcs must be 1:1 vs 1:many/none, vs structs etc).

Not necessary to do it in this PR.

@lostluck, the space will not create any problem. It is friendlier for the learner to see the lesson and task names in the natural way versus using underscore name.

Co-authored-by: Robert Burke <lostluck@users.noreply.github.com>

henryken

This LGTM now.

henryken · 2020-05-05T00:46:54Z

Please wait for the course uploading before merging this PR.

damondouglas · 2020-05-05T02:12:14Z

@henryken and @lostluck I updated the Stepik course and commited the *-remote-info.yaml files. Thank you both for your help.

henryken · 2020-05-05T03:01:30Z

Awesome! This PR can be merged now. Thanks @damondouglas!

lostluck · 2020-05-07T22:27:11Z

Retest this please

…tas (apache#11564)

Damon Douglas and others added 18 commits April 18, 2020 10:09

Scaffold Core Transforms Map lesson

2992d0b

Write Core Transforms Map ParDoTtask information

22976ed

Write Core Transforms Map ParDo OneToMany task information

3ee6eeb

Write Core Transforms Map MapElements task information

5059435

Write Core Transforms Map FlatMapElements task information

4f3c4aa

Create Core Transforms Map ParDo task

c5656c5

Change placeholder_text to TODO()

ac8ede3

WIP: complete ParDo OneToMany, MapElements tasks without placeholder

ffab139

Remove reduntant test files

2095500

Add answer placeholder for ParDo OneToMany

7334686

Add answer placeholder for MapElements

d9488c7

Reorganize and edit task information

1d58872

Complete MapElements task

d146488

Create FlatMapElements task

ca29422

Make go.sum invisible

33498e5

Add main to MapElements

7beada6

Edit namings to be consistent

500ff71

Move placeholders to be more legible

c9d26a5

probot-autolabeler bot added the learning label Apr 29, 2020

henryken suggested changes Apr 30, 2020

View reviewed changes

Update per PR review suggestions

9c5d3b2

henryken suggested changes May 1, 2020

View reviewed changes

Damon Douglas added 2 commits May 3, 2020 11:54

Change task name and placeholder coverage

373adc2

Change task title

1bb2c30

henryken suggested changes May 4, 2020

View reviewed changes

learning/katas/go/Core Transforms/Map/ParDo OneToMany/pkg/task/task.go Show resolved Hide resolved

learning/katas/go/Core Transforms/Map/ParDo struct/pkg/task/task.go Outdated Show resolved Hide resolved

Delete and recreate ParDo Struct

9827542

lostluck approved these changes May 4, 2020

View reviewed changes

Update learning/katas/go/Core Transforms/Map/ParDo OneToMany/task.md

5c8e3db

Co-authored-by: Robert Burke <lostluck@users.noreply.github.com>

henryken approved these changes May 5, 2020

View reviewed changes

Upload course updates to stepik

099c42b

lostluck merged commit e6fb32e into apache:master May 7, 2020

lostluck changed the title ~~[Beam-9679] Add Core Transforms section / Map lesson to the Go SDK katas~~ [Beam-9679] Add Core Transforms section / ParDo lesson to the Go SDK katas May 7, 2020

damondouglas deleted the BEAM-9679-core-transforms-map-kata-go branch May 13, 2020 03:07

damondouglas mentioned this pull request May 13, 2020

[BEAM-9928] Add qualified import name to task descriptors #11690

Merged

4 tasks

yirutang pushed a commit to yirutang/beam that referenced this pull request Jul 23, 2020

[Beam-9679] Add Core Transforms section / Map lesson to the Go SDK ka…

30e7a04

…tas (apache#11564)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Beam-9679] Add Core Transforms section / ParDo lesson to the Go SDK katas #11564

[Beam-9679] Add Core Transforms section / ParDo lesson to the Go SDK katas #11564

damondouglas commented Apr 29, 2020 •

edited

lostluck commented Apr 29, 2020

lostluck commented Apr 29, 2020

henryken left a comment

damondouglas commented May 1, 2020

damondouglas commented May 1, 2020

henryken left a comment

damondouglas commented May 3, 2020

henryken left a comment

damondouglas commented May 4, 2020

lostluck commented May 4, 2020

lostluck commented May 4, 2020

damondouglas commented May 4, 2020 •

edited by lostluck

lostluck left a comment

lostluck May 4, 2020

damondouglas May 5, 2020

lostluck May 7, 2020

lostluck May 7, 2020

damondouglas May 7, 2020

lostluck May 4, 2020

henryken May 5, 2020

henryken left a comment

henryken commented May 5, 2020

damondouglas commented May 5, 2020

henryken commented May 5, 2020

lostluck commented May 7, 2020

[Beam-9679] Add Core Transforms section / ParDo lesson to the Go SDK katas #11564

[Beam-9679] Add Core Transforms section / ParDo lesson to the Go SDK katas #11564

Conversation

damondouglas commented Apr 29, 2020 • edited

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

lostluck commented Apr 29, 2020

lostluck commented Apr 29, 2020

henryken left a comment

Choose a reason for hiding this comment

damondouglas commented May 1, 2020

damondouglas commented May 1, 2020

henryken left a comment

Choose a reason for hiding this comment

damondouglas commented May 3, 2020

henryken left a comment

Choose a reason for hiding this comment

damondouglas commented May 4, 2020

lostluck commented May 4, 2020

lostluck commented May 4, 2020

damondouglas commented May 4, 2020 • edited by lostluck

lostluck left a comment

Choose a reason for hiding this comment

lostluck May 4, 2020

Choose a reason for hiding this comment

damondouglas May 5, 2020

Choose a reason for hiding this comment

lostluck May 7, 2020

Choose a reason for hiding this comment

lostluck May 7, 2020

Choose a reason for hiding this comment

damondouglas May 7, 2020

Choose a reason for hiding this comment

lostluck May 4, 2020

Choose a reason for hiding this comment

henryken May 5, 2020

Choose a reason for hiding this comment

henryken left a comment

Choose a reason for hiding this comment

henryken commented May 5, 2020

damondouglas commented May 5, 2020

henryken commented May 5, 2020

lostluck commented May 7, 2020

damondouglas commented Apr 29, 2020 •

edited

damondouglas commented May 4, 2020 •

edited by lostluck