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/06/10 22:40:39 UTC
[GitHub] [beam] damondouglas opened a new pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
damondouglas opened a new pull request #11979:
URL: https://github.com/apache/beam/pull/11979
This pull requests adds a Partition lesson to the Go SDK katas. I would like to request the following reviewers:
(R: @lostluck )
(R: @henryken )
If accepted by both reviewers, please wait until the [Stepik course](https://stepik.org/course/70387) is updated before finally merging this PR.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [x] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
- [x] 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 | Apex | Dataflow | Flink | Samza | Spark
--- | --- | --- | --- | --- | --- | ---
Go | [](https://builds.apache.org/job/beam_PostCommit_Go/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Flink/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Go_VR_Spark/lastCompletedBuild/)
Java | [](https://builds.apache.org/job/beam_PostCommit_Java/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Apex/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Dataflow_Java11/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Flink_Java11/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Streaming/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Samza/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_PVR_Spark_Batch/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming/lastCompletedBuild/)
Python | [](https://builds.apache.org/job/beam_PostCommit_Python2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python36/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python37/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_VR_Dataflow_V2/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Py_ValCont/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python2_PVR_Flink_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PostCommit_Python35_VR_Flink/lastCompletedBuild/) | --- | [](https://builds.apache.org/job/beam_PostCommit_Python_VR_Spark/lastCompletedBuild/)
XLang | --- | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Flink/lastCompletedBuild/) | --- | --- | [](https://builds.apache.org/job/beam_PostCommit_XVR_Spark/lastCompletedBuild/)
Pre-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
--- |Java | Python | Go | Website
--- | --- | --- | --- | ---
Non-portable | [](https://builds.apache.org/job/beam_PreCommit_Java_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Python_Cron/lastCompletedBuild/)<br>[](https://builds.apache.org/job/beam_PreCommit_PythonLint_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Go_Cron/lastCompletedBuild/) | [](https://builds.apache.org/job/beam_PreCommit_Website_Cron/lastCompletedBuild/)
Portable | --- | [](https://builds.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.
----------------------------------------------------------------
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] damondouglas commented on pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-655901876
@henryken / @lostluck I've started work on consolidating the module by beginning work at this branch: https://github.com/apache/beam/compare/master...damondouglas:BEAM-10428-single-module-kata-go
Essentially the final goal is to make one go.mod at the root of learning/katas/go. So far I've only changed the Hello Beam section so you can preview what I intend for the rest.
Would it make sense to start a PR for this branch without the intent to merge initially?
----------------------------------------------------------------
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] damondouglas commented on a change in pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11979:
URL: https://github.com/apache/beam/pull/11979#discussion_r439522967
##########
File path: learning/katas/go/Core Transforms/Partition/Partition/pkg/task/task.go
##########
@@ -0,0 +1,27 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package task
+
+import "github.com/apache/beam/sdks/go/pkg/beam"
+
+func ApplyTransform(s beam.Scope, input beam.PCollection) []beam.PCollection {
Review comment:
My thought was to alert the individual that the return type is different than previous tasks. Yet, they would be alerted anyway if specifying `beam.PCollection` instead of `[]beam.PCollection`. I removed 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] damondouglas edited a comment on pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
damondouglas edited a comment on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-655901876
@henryken / @lostluck I've started work on consolidating the module by beginning work at this branch: https://github.com/apache/beam/compare/master...damondouglas:BEAM-10428-single-module-kata-go
Essentially the final goal is to make one go.mod at the root of learning/katas/go. So far I've only changed the Hello Beam section so you can preview what I intend for the rest.
Would it make sense to start a PR for this branch without the intent to merge initially but until the entire learning/katas/go is refactored?
----------------------------------------------------------------
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] henryken commented on pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
henryken commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-654574672
The Kata generated metadata files are important and only contain few lines of code.
What I understand is that each lesson is run like a mini Go project/module.
@damondouglas, you can give a try having one go.mod file for all lessons, if it's possible.
----------------------------------------------------------------
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 #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-655910522
That works fine. Given how mechanical the change is, it wouldn't be unreasonable to do the change all at once in one PR.
However, I do recommend doing all the directory renames in separate commits from the deleting the old mods so that we can check the relatively smaller changes on a per commit basis (rather than just all at once). Won't matter to the final merge, since it will get squashed, but it will make it easier to review.
----------------------------------------------------------------
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] damondouglas commented on pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-643045181
@henryken should I go ahead and update the Stepik course?
----------------------------------------------------------------
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] damondouglas commented on pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-654572661
IntelliJ Edu autogenerated a go.mod in each lesson so that they are independently runnable when the course files are provided to the user. I had been thinking of deviating from IntelliJ Edu's method and instead just having one go.mod in learning/katas/go. Should I start another pull request if I'm successful at this?
----------------------------------------------------------------
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] damondouglas commented on pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-643713390
@lostluck / @henryken Thank you for reviewing. I'll update the stepik course and provide the updated `*-remote.yaml` files in the next PR planned for Side Input.
----------------------------------------------------------------
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 #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
lostluck commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-654457555
I can't speak to the Kata generated code, but the go.mod and go.sum files are strongly recommended to be submitted as is to ensure full transitive dependency of the right and correct package versions are included. They aren't intended for human consumption for the most part.
----------------------------------------------------------------
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] damondouglas commented on a change in pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
damondouglas commented on a change in pull request #11979:
URL: https://github.com/apache/beam/pull/11979#discussion_r439471700
##########
File path: learning/katas/go/Core Transforms/Partition/Partition/go.mod
##########
@@ -0,0 +1,27 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+module partition
+
+go 1.14
+
+require (
+ github.com/apache/beam v2.22.0+incompatible
Review comment:
Thank you, @henryken for picking up on that. I will add this to my checklist to remind me for every 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] henryken commented on a change in pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11979:
URL: https://github.com/apache/beam/pull/11979#discussion_r439190971
##########
File path: learning/katas/go/Core Transforms/Partition/Partition/go.mod
##########
@@ -0,0 +1,27 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+module partition
+
+go 1.14
+
+require (
+ github.com/apache/beam v2.22.0+incompatible
Review comment:
I realize that we're using multiple versions of Beam, from v2.19 to v2.22.
Can we standardize to just one version for the entire course?
##########
File path: learning/katas/go/Core Transforms/Partition/Partition/pkg/task/task.go
##########
@@ -0,0 +1,27 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package task
+
+import "github.com/apache/beam/sdks/go/pkg/beam"
+
+func ApplyTransform(s beam.Scope, input beam.PCollection) []beam.PCollection {
Review comment:
Just would like to check whether you intend to have an answer placeholder for the return value?
In other tasks, we normally don't ask 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] henryken commented on a change in pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
henryken commented on a change in pull request #11979:
URL: https://github.com/apache/beam/pull/11979#discussion_r439725986
##########
File path: learning/katas/go/Core Transforms/Partition/Partition/pkg/task/task.go
##########
@@ -0,0 +1,27 @@
+// Licensed to the Apache Software Foundation (ASF) under one or more
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership.
+// The ASF licenses this file to You under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package task
+
+import "github.com/apache/beam/sdks/go/pkg/beam"
+
+func ApplyTransform(s beam.Scope, input beam.PCollection) []beam.PCollection {
Review comment:
👌
----------------------------------------------------------------
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 #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
lostluck merged pull request #11979:
URL: https://github.com/apache/beam/pull/11979
----------------------------------------------------------------
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] robertwb commented on pull request #11979: [BEAM-9679] Add Partition task to Core Transform katas
Posted by GitBox <gi...@apache.org>.
robertwb commented on pull request #11979:
URL: https://github.com/apache/beam/pull/11979#issuecomment-654416116
I am surprised to see a 859 line PR for what amounts to a 20-line example accompanied by about 20 lines of docs. Is there some way to have less auto-generated code checked in?
----------------------------------------------------------------
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