[BEAM-1542] SpannerIO sink updates #3221

mairbek · 2017-05-24T22:15:52Z

Unit and integration test.
Logical mutation size estimation.

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

Make sure the PR title is formatted like:
[BEAM-<Jira issue #>] Description of pull request
Make sure tests pass via mvn clean verify.
Replace <Jira issue #> in the title with the actual Jira issue
number, if there is one.
If this contribution is large, please file an Apache
Individual Contributor License Agreement.

This closes apache#2166.

Also minor cleanup alphabetization in root pom.xml

This closes apache#2166.

Also minor cleanup alphabetization in root pom.xml

This is not appropriate for examples. SpannerIO should be well-javadoced and integration tested.

And remove outdated Bigtable comment

* Rename to Write to match the rest of the SDK. * Convert to AutoValue, delete toString. * Drop .writeTo(), instead use .write() as default constructor. * Temporarily drop withBatchSize, as its existence is not clearly justified.

… mergespanner # Conflicts: # sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java # sdks/java/io/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/GcpApiSurfaceTest.java

* Rename to Write to match the rest of the SDK. * Convert to AutoValue, delete toString. * Drop .writeTo(), instead use .write() as default constructor. * Temporarily drop withBatchSize, as its existence is not clearly justified.

Better formatting

Resolved merge

…nner # Conflicts: # pom.xml # sdks/java/io/google-cloud-platform/pom.xml # sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

coveralls · 2017-05-25T00:29:37Z

Coverage increased (+0.02%) to 70.723% when pulling 18c1ab6 on mairbek:mergespanner into c0d19f9 on apache:master.

dhalperi

Looking good. I'll run the postcommit to make sure the integration test actually passes.

dhalperi · 2017-05-25T16:04:09Z

...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

 */
 @Experimental(Experimental.Kind.SOURCE_SINK)
 public class SpannerIO {

-  @VisibleForTesting
-  static final int SPANNER_MUTATIONS_PER_COMMIT_LIMIT = 20000;
+  private static final long DEFAULT_BATCH_SIZE = 1024 * 1024; // 1 MB


please add _BYTES here and to the relevant builder.

dhalperi · 2017-05-25T16:04:23Z

...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

@@ -72,21 +75,32 @@
 * mutations.apply(
 *     "Write", SpannerIO.write().withInstanceId("instance").withDatabaseId("database"));
 * }</pre>
+ *
+ * <p>The default size of the batch is set to 1MB, to override this use {@link
+ * Write#withBatchSize(long)}. Setting batch size to a small value or zero practically disables


withBatchSizeBytes

dhalperi · 2017-05-25T16:06:29Z

...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

@@ -217,20 +282,19 @@ public void finishBundle() throws Exception {
    @Teardown
    public void teardown() throws Exception {
      if (spanner == null) {
-          return;
+        return;
      }
      spanner.closeAsync().get();


set spanner to null here?

dhalperi · 2017-05-25T16:08:06Z

...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

-          .addIfNotNull(DisplayData.item("databaseId", databaseId)
-              .withLabel("Database"));
+          .addIfNotNull(DisplayData.item("instanceId", spec.getInstanceId()).withLabel("Instance"))
+          .addIfNotNull(DisplayData.item("databaseId", spec.getDatabaseId()).withLabel("Database"));


add if not default batchsize

add the service factory or the class of it?

dhalperi · 2017-05-25T16:09:17Z

...o/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerWriteIT.java

+    @Description("Instance ID to write to in Spanner")
+    @Default.String("beam-test")
+    String getInstanceId();
+


please delete empty lines between getter and setter in options.

dhalperi · 2017-05-25T16:10:02Z

...o/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerWriteIT.java

+                    + " ("
+                    + "  Key           INT64,"
+                    + "  Value         STRING(MAX),"
+                    + ") PRIMARY KEY (Key)"));


is there a way in the spanner client to defend against injection attacks?

Not with DDL...

dhalperi · 2017-05-25T16:13:43Z

https://builds.apache.org/view/Beam/job/beam_PostCommit_Java_MavenInstall/3916/ is a run of the postcommit against this PR

aaltay · 2017-05-25T16:42:34Z

R: @dhalperi

dhalperi · 2017-05-25T17:12:47Z

...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

        flushBatch();
      }
    }

+    private String projectId() {
+      return spec.getProjectId() == null
+          ? ServiceOptions.getDefaultProjectId()


@lukecwik you know a lot about default projects -- does this do the right thing generally? Should we switch to it at a larger scale?

I wanted to swap to use ServiceOptions.getDefaultProjectId() a while ago and have not been able to perform that migration for GcpOptions. If we can include it here great. This would remove all our hand-coded detect default project code. Note that by doing this we would remove support for gcloud users who have old gcloud SDK installations.

I requested the original change in gcloud-java-core to have this exposed:
googleapis/google-cloud-java#1380

dhalperi · 2017-05-25T18:00:46Z

...o/google-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerWriteIT.java

+    spanner = SpannerOptions.newBuilder().setProjectId(options.getProjectId()).build().getService();
+
+    databaseAdminClient = spanner.getDatabaseAdminClient();
+    Operation<Database, CreateDatabaseMetadata> op =


Imagining failure modes, let's say for some reason the tearDown step does not run. We should try dropping the database here.

Makes sense, added a line to to drop the DB.
In case if DB is not found it's a noop.

dhalperi · 2017-05-25T18:35:27Z

https://builds.apache.org/view/Beam/job/beam_PostCommit_Java_MavenInstall/3919/ is a run of the ITs against the current HEAD

dhalperi · 2017-05-26T17:11:10Z

retest this please

dhalperi · 2017-05-26T18:16:56Z

Can you rebase and squash to a single commit? I'm still not sure why the build is failing

coveralls · 2017-05-26T18:49:20Z

Changes Unknown when pulling bd3912b8086eaf100abe971d83a2b9172a0c05f8 on mairbek:mergespanner into ** on apache:master**.

dhalperi · 2017-05-26T19:33:34Z

https://builds.apache.org/view/Beam/job/beam_PostCommit_Java_MavenInstall/3935/ is a run of postcommit against current branch

coveralls · 2017-05-26T19:54:34Z

Coverage decreased (-0.003%) to 70.794% when pulling d20d6da on mairbek:mergespanner into c687887 on apache:master.

coveralls · 2017-05-26T20:08:22Z

Coverage remained the same at 70.797% when pulling d20d6da on mairbek:mergespanner into c687887 on apache:master.

dhalperi · 2017-05-26T20:09:32Z

...ava/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

+              DisplayData.item("instanceId", getInstanceId()).withLabel("Output Instance"))
+          .addIfNotNull(
+              DisplayData.item("databaseId", getDatabaseId()).withLabel("Output Database"))
+          .addIfNotNull(


this can't be null, it's a long.

Use addIfNotDefault so it doesn't show up when left to default settings. https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/display/DisplayData.java#L212

I think it is valuable to keep it. -> using add.

dhalperi · 2017-05-26T20:11:25Z

The IT passed, so LGTM. One minor recommendation left.

coveralls · 2017-05-26T20:52:44Z

Coverage decreased (-0.01%) to 70.787% when pulling c07a32f on mairbek:mergespanner into c687887 on apache:master.

coveralls · 2017-05-26T21:38:36Z

Coverage decreased (-0.01%) to 70.783% when pulling c07a32f on mairbek:mergespanner into c687887 on apache:master.

gamolina and others added 25 commits May 4, 2017 11:28

Initial implementation of SpannerIO.Write

8db6693

This closes apache#2166.

Minor style, compilation, javadoc fixups

491f047

SpannerCSVLoader: update for TextIO API Change

7a2d09a

Fix spanner dependency management

77be276

Also minor cleanup alphabetization in root pom.xml

Introduced MutationSizeEstimator.

4049047

Initial implementation of SpannerIO.Write

4392de9

This closes apache#2166.

Minor style, compilation, javadoc fixups

b092be8

Fix spanner dependency management

e16339a

Also minor cleanup alphabetization in root pom.xml

Delete SpannerCSVLoader

9798534

This is not appropriate for examples. SpannerIO should be well-javadoced and integration tested.

Refine Spanner API tests

f495a41

And remove outdated Bigtable comment

SpannerIO.Write cleanup and style fixes

ce5d04c

* Rename to Write to match the rest of the SDK. * Convert to AutoValue, delete toString. * Drop .writeTo(), instead use .write() as default constructor. * Temporarily drop withBatchSize, as its existence is not clearly justified.

Use a new batch size limit.

16e4b83

SpannerIO.Write cleanup and style fixes

5a2214a

* Rename to Write to match the rest of the SDK. * Convert to AutoValue, delete toString. * Drop .writeTo(), instead use .write() as default constructor. * Temporarily drop withBatchSize, as its existence is not clearly justified.

Added a SpannerIO unit test.

5fc07fe

Renaming and documenting

0b7b84d

Reorganized test.

2b5a10e

Better formatting

Merge remote-tracking branch 'dhalperi/takeover-2166' into mergespanner

938b92c

Resolved merge

Pre PR cleanup

fd2a76c

Sketch out integration test

e24254c

End to end test for Cloud Spanner Sink

34c7c6f

Make checkstyle happy

f30b520

Merge branch 'master' of https://github.com/apache/beam into mergespa…

060ff58

…nner # Conflicts: # pom.xml # sdks/java/io/google-cloud-platform/pom.xml # sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

Merge branch 'master' of https://github.com/apache/beam into mergespa…

1f256ef

…nner # Conflicts: # pom.xml # sdks/java/io/google-cloud-platform/pom.xml # sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIO.java

Revert style change

dabda8d

dhalperi requested changes May 25, 2017

View reviewed changes

dhalperi reviewed May 25, 2017

View reviewed changes

mairbek force-pushed the mergespanner branch 2 times, most recently from 7af3a76 to bd3912b Compare May 26, 2017 18:32

mairbek force-pushed the mergespanner branch from bd3912b to d20d6da Compare May 26, 2017 19:19

dhalperi reviewed May 26, 2017

View reviewed changes

Addressing code review comments.

c07a32f

mairbek force-pushed the mergespanner branch from d20d6da to c07a32f Compare May 26, 2017 20:17

dhalperi mentioned this pull request May 26, 2017

[BEAM-1542] SpannerIO sink updates #3241

Merged

4 tasks

mairbek closed this May 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-1542] SpannerIO sink updates #3221

[BEAM-1542] SpannerIO sink updates #3221

mairbek commented May 24, 2017

coveralls commented May 25, 2017

dhalperi left a comment

dhalperi May 25, 2017

dhalperi May 25, 2017

mairbek May 25, 2017

dhalperi May 25, 2017

mairbek May 25, 2017

dhalperi May 25, 2017

mairbek May 25, 2017

dhalperi May 25, 2017

dhalperi May 25, 2017

mairbek May 25, 2017

dhalperi commented May 25, 2017

aaltay commented May 25, 2017

dhalperi May 25, 2017

lukecwik May 25, 2017

dhalperi May 25, 2017

mairbek May 25, 2017

dhalperi commented May 25, 2017

dhalperi commented May 26, 2017

dhalperi commented May 26, 2017

coveralls commented May 26, 2017

dhalperi commented May 26, 2017

coveralls commented May 26, 2017

coveralls commented May 26, 2017

dhalperi May 26, 2017

mairbek May 26, 2017

dhalperi commented May 26, 2017

coveralls commented May 26, 2017

coveralls commented May 26, 2017

[BEAM-1542] SpannerIO sink updates #3221

[BEAM-1542] SpannerIO sink updates #3221

Conversation

mairbek commented May 24, 2017

coveralls commented May 25, 2017

dhalperi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhalperi commented May 25, 2017

aaltay commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhalperi commented May 25, 2017

dhalperi commented May 26, 2017

dhalperi commented May 26, 2017

coveralls commented May 26, 2017

dhalperi commented May 26, 2017

coveralls commented May 26, 2017

coveralls commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhalperi commented May 26, 2017

coveralls commented May 26, 2017

coveralls commented May 26, 2017