Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BEAM-9679] Add a windowed PCollection lesson to Go SDK Katas #12448

Merged
merged 4 commits into from Aug 6, 2020
Merged

[BEAM-9679] Add a windowed PCollection lesson to Go SDK Katas #12448

merged 4 commits into from Aug 6, 2020

Conversation

damondouglas
Copy link
Contributor

@damondouglas damondouglas commented Aug 2, 2020 •
edited

This pull request adds a windowed PCollection lesson to the Go SDK katas. I would like to request the following reviewer:

(R: @lostluck )

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 Dataflow Flink Samza Spark Twister2
Go Build Status --- Build Status --- Build Status ---
Java Build Status Build Status
Build Status		 Build Status
Build Status
Build Status
Build Status		 Build Status Build Status
Build Status
Build Status		 Build Status
Python Build Status
Build Status
Build Status
Build Status
Build Status		 Build Status
Build Status
Build Status		 Build Status
Build Status		 --- Build Status ---
XLang Build Status --- Build Status --- Build Status ---

Pre-Commit Tests Status (on master branch)

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

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

lostluck
lostluck requested changes Aug 3, 2020
View reviewed changes
Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm expecting to go back and forth a few rounds on the text here. Partly teaching, partly cleanup. Please do ask any questions if I'm being confusing.

learning/katas/go/core_transforms/additional_parameters/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/additional_parameters/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/additional_parameters/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/additional_parameters/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/additional_parameters/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/additional_parameters/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/additional_parameters/additional_parameters/task.md Outdated Show resolved Hide resolved
lostluck
lostluck requested changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few more edits to the text, but otherwise looks good!

I note that this "additional parameters" lesson is now under a windowing section. I assume there might be an additional lessons to cover the other basic windowing types, sliding, and session? I like that as a flow, show the mechanics of the basics, show the other options...
Granted, session isn't fully implemented in the SDK yet, let alone custom windowing...

learning/katas/go/core_transforms/windowing/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/windowing/additional_parameters/task.md Outdated Show resolved Hide resolved
learning/katas/go/core_transforms/windowing/additional_parameters/task.md Outdated Show resolved Hide resolved
@damondouglas damondouglas changed the title [BEAM-9679] Add Additional Parameters lesson to Go SDK Katas [BEAM-9679] Add a windowed PCollection lesson to Go SDK Katas Aug 6, 2020
lostluck
lostluck approved these changes Aug 6, 2020
View reviewed changes
Copy link
Contributor

@lostluck lostluck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Let me know when Stepik is updated and I'll merge.
Thank you!

@damondouglas
Copy link
Contributor Author

@lostluck the stepik course has been updated and the *-remote.yaml files committed to this PR. This PR is ready to merge. Thank you so much for patiently walking with me through this.

@lostluck
Copy link
Contributor

lostluck commented Aug 6, 2020

No worries! You're getting to the point where we need more features in the SDK before more Katas can be written :D

@lostluck lostluck merged commit 064e6ba into apache:master Aug 6, 2020
@damondouglas damondouglas deleted the BEAM-9679-core-transforms-addl-param branch August 8, 2020 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lostluck lostluck lostluck approved these changes

Assignees
No one assigned
Labels
learning
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@damondouglas @lostluck