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/26 21:05:32 UTC
[GitHub] [beam] chamikaramj commented on a change in pull request #12071: [BEAM-9932] Add documentation describing cross-language test pipelines
chamikaramj commented on a change in pull request #12071:
URL: https://github.com/apache/beam/pull/12071#discussion_r445925959
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -110,6 +137,20 @@ private void waitForReady() {
}
}
+ /**
+ * Motivation behind <i>singleInputOutputTest</i>.
+ *
+ * <ul>
+ * <li><b>Target transform</b> – {@link ParDo}
Review comment:
Lets link to website instead of Java version. For example, https://beam.apache.org/documentation/programming-guide/#pardo
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -120,6 +161,20 @@ public void singleInputOutputTest() throws IOException {
PAssert.that(col).containsInAnyOrder("01", "02", "03");
}
+ /**
+ * Motivation behind <i>multiInputOutputWithSideInputTest</i>.
+ *
+ * <ul>
+ * <li><b>Target transform</b> – {@link ParDo}
Review comment:
Ditto.
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -110,6 +137,20 @@ private void waitForReady() {
}
}
+ /**
+ * Motivation behind <i>singleInputOutputTest</i>.
+ *
+ * <ul>
+ * <li><b>Target transform</b> – {@link ParDo}
+ * <li><b>Test scenario</b> – Mapping elements from a single input collection to a single output
+ * collection
+ * <li><b>Boundary conditions checked</b> –
+ * <ul>
+ * <li>PCollection<?> to external transforms
Review comment:
Lets refer to following fo PCollection.
https://beam.apache.org/documentation/programming-guide/#pcollections
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -187,6 +283,20 @@ public void combineGloballyTest() {
PAssert.that(col).containsInAnyOrder(6L);
}
+ /**
+ * Motivation behind <i>combinePerKeyTest</i>.
Review comment:
Lets refer to https://beam.apache.org/documentation/programming-guide/#combine
##########
File path: sdks/python/apache_beam/transforms/validate_runner_xlang_test.py
##########
@@ -14,6 +14,38 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
+
+"""
+###########################################################
+Runner Validation Test Suite for Cross-language Transforms
+###########################################################
+As per Beams's Portability Framework design, Cross-language transforms should
+work out of the box. In spite of this, there always exists a possibility of
Review comment:
Same as above, change to something more direct like following.
Cross-language transforms runner validation test suite for Java SDK.
These tests check whether core Beam transforms work correctly when used as cross-language transforms from Java SDK. This is needed to make sure that cross-language transforms framework works correctly for various corner cases. Please see documentation for individual tests for more details regarding cases covered by each test. Please see here<link to doc> for more details.
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -154,6 +223,20 @@ public void groupByKeyTest() {
PAssert.that(col).containsInAnyOrder("0:1,2", "1:3");
}
+ /**
+ * Motivation behind <i>coGroupByKeyTest</i>.
+ *
+ * <ul>
+ * <li><b>Target transform</b> – {@link CoGroupByKey}
Review comment:
Lets refer to https://beam.apache.org/documentation/programming-guide/#cogroupbykey
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -177,6 +260,19 @@ public void coGroupByKeyTest() {
PAssert.that(col).containsInAnyOrder("0:1,2,4", "1:3,5,6");
}
+ /**
+ * Motivation behind <i>combineGloballyTest</i>.
Review comment:
Lets refer to https://beam.apache.org/documentation/programming-guide/#combine
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -197,6 +307,19 @@ public void combinePerKeyTest() {
PAssert.that(col).containsInAnyOrder(KV.of("a", 3L), KV.of("b", 3L));
}
+ /**
+ * Motivation behind <i>flattenTest</i>.
Review comment:
Lets refer to https://beam.apache.org/documentation/programming-guide/#flatten
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -54,15 +60,36 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Test External transforms. */
+/**
+ * Runner Validation Test Suite for Cross-language Transforms.
+ *
+ * <p>As per Beams's Portability Framework design, Cross-language transforms should work out of the
+ * box. In spite of this, there always exists a possibility of rough edges existing. It could be
+ * caused due to unpolished implementation of any part of the execution code path, for example:
+ *
+ * <ul>
+ * <li>Transform expansion [SDK]
+ * <li>Pipeline construction [SDK]
+ * <li>Cross-language artifact staging [Runner]
+ * <li>Language specific serialization/deserialization of PCollection (and other data types)
+ * [Runner/SDK]
+ * </ul>
+ *
+ * <p>In an effort to improve developer visibility into potential problems, this test suite
+ * validates a cross-language runner against <i>5 Core Beam transforms</i> from any foreign language
Review comment:
These tests actually tests whether versions of these transforms in foreign SDKs work from Java. I don't think we need to refer to Java versions here. We can just just mention that we test "core Beam transforms" here and link to Website below when possible without referring to (or importing) Java versions.
##########
File path: sdks/python/apache_beam/transforms/validate_runner_xlang_test.py
##########
@@ -14,6 +14,38 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
+
+"""
+###########################################################
+Runner Validation Test Suite for Cross-language Transforms
+###########################################################
+As per Beams's Portability Framework design, Cross-language transforms should
+work out of the box. In spite of this, there always exists a possibility of
+rough edges existing. It could be caused due to unpolished implementation of any
+part of the execution code path, for example:
+
+* Transform expansion [SDK]
Review comment:
Lets refer to same Website links here as well for corresponding transforms.
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -54,15 +60,36 @@
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;
-/** Test External transforms. */
+/**
+ * Runner Validation Test Suite for Cross-language Transforms.
+ *
+ * <p>As per Beams's Portability Framework design, Cross-language transforms should work out of the
Review comment:
I think this can be replaced by something like following which gives a more direct explanation:
Cross-language transforms runner validation test suite for Java SDK.
These tests check whether core Beam transforms work correctly when used as cross-language transforms from Java SDK. This is needed to make sure that cross-language transforms framework works correctly for various corner cases. Please see documentation for individual tests for more details regarding cases covered by each test. Please see here<link to doc> for more details.
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -209,6 +332,20 @@ public void flattenTest() {
PAssert.that(col).containsInAnyOrder(1L, 2L, 3L, 4L, 5L, 6L);
}
+ /**
+ * Motivation behind <i>partitionTest</i>.
Review comment:
Lets refer to https://beam.apache.org/documentation/programming-guide/#partition
##########
File path: runners/core-construction-java/src/test/java/org/apache/beam/runners/core/construction/ValidateRunnerXlangTest.java
##########
@@ -135,6 +190,20 @@ public void multiInputOutputWithSideInputTest() {
PAssert.that(pTuple.get("side")).containsInAnyOrder("ss");
}
+ /**
+ * Motivation behind <i>groupByKeyTest</i>.
+ *
+ * <ul>
+ * <li><b>Target transform</b> – {@link GroupByKey}
Review comment:
Lets refer to https://beam.apache.org/documentation/programming-guide/#groupbykey
----------------------------------------------------------------
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