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/04/26 16:31:40 UTC
[GitHub] [beam] nielm opened a new pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
nielm opened a new pull request #11528:
URL: https://github.com/apache/beam/pull/11528
Not all of the batching/grouping parameters are added in populateDisplayData() for SpannerIO.Write, and there is no displayData in SpannerIO.WriteGrouped.
This PR adds all the grouping/batching parameters to both
`SpannerIO.Write.populateDisplayData()`
`SpannerIO.WriteGrouped.populateDisplayData`
Post-Commit Tests Status (on master branch)
------------------------------------------------------------------------------------------------
Lang | SDK | Apex | Dataflow | Flink | Gearpump | 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_Gearpump/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] TheNeuralBit commented on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-630494783
Retest this please
----------------------------------------------------------------
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] nielm removed a comment on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
nielm removed a comment on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-630469413
Retest this please
----------------------------------------------------------------
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] nielm commented on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
nielm commented on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-619911716
retest this please
----------------------------------------------------------------
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] nielm commented on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
nielm commented on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-630469413
Retest this please
----------------------------------------------------------------
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] nielm commented on a change in pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
nielm commented on a change in pull request #11528:
URL: https://github.com/apache/beam/pull/11528#discussion_r426926639
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java
##########
@@ -991,6 +1001,24 @@ public WriteGrouped(Write spec) {
this.spec = spec;
}
+ @Override
+ public void populateDisplayData(DisplayData.Builder builder) {
+ super.populateDisplayData(builder);
+ spec.getSpannerConfig().populateDisplayData(builder);
+ builder.add(
+ DisplayData.item("batchSizeBytes", spec.getBatchSizeBytes())
+ .withLabel("Max batch size in sytes"));
+ builder.add(
+ DisplayData.item("maxNumMutations", spec.getMaxNumMutations())
+ .withLabel("Max number of mutated cells in each batch"));
+ builder.add(
+ DisplayData.item("maxNumRows", spec.getMaxNumRows())
+ .withLabel("Max number of rows in each batch"));
+ builder.add(
+ DisplayData.item("groupingFactor", spec.getGroupingFactor())
+ .withLabel("Number of batches to sort over"));
+ }
+
Review comment:
LGTM.
I extracted a single method in Write to populate the displayData with parameters to avoid repeating code.
----------------------------------------------------------------
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] allenpradeep commented on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
allenpradeep commented on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-622521247
LGTM. 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [beam] nielm commented on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
nielm commented on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-630492150
Retest this please
----------------------------------------------------------------
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] TheNeuralBit commented on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-630494999
Retest this please
----------------------------------------------------------------
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] TheNeuralBit merged pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
TheNeuralBit merged pull request #11528:
URL: https://github.com/apache/beam/pull/11528
----------------------------------------------------------------
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] TheNeuralBit commented on pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on pull request #11528:
URL: https://github.com/apache/beam/pull/11528#issuecomment-630470331
Retest this please
----------------------------------------------------------------
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] TheNeuralBit commented on a change in pull request #11528: [BEAM-9821] Populate all SpannerIO batching parameters in display data.
Posted by GitBox <gi...@apache.org>.
TheNeuralBit commented on a change in pull request #11528:
URL: https://github.com/apache/beam/pull/11528#discussion_r425385260
##########
File path: sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java
##########
@@ -991,6 +1001,24 @@ public WriteGrouped(Write spec) {
this.spec = spec;
}
+ @Override
+ public void populateDisplayData(DisplayData.Builder builder) {
+ super.populateDisplayData(builder);
+ spec.getSpannerConfig().populateDisplayData(builder);
+ builder.add(
+ DisplayData.item("batchSizeBytes", spec.getBatchSizeBytes())
+ .withLabel("Max batch size in sytes"));
+ builder.add(
+ DisplayData.item("maxNumMutations", spec.getMaxNumMutations())
+ .withLabel("Max number of mutated cells in each batch"));
+ builder.add(
+ DisplayData.item("maxNumRows", spec.getMaxNumRows())
+ .withLabel("Max number of rows in each batch"));
+ builder.add(
+ DisplayData.item("groupingFactor", spec.getGroupingFactor())
+ .withLabel("Number of batches to sort over"));
+ }
+
Review comment:
I added some commits to tweak these descriptions a bit. @nielm can you confirm that they're still correct?
Also I wonder if you can re-use the implementation in `Write` by calling `spec.populateDisplayData(builder)` here instead?
----------------------------------------------------------------
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