[BEAM-1542] Cloud Spanner Source #3395

mairbek · 2017-06-19T20:39:15Z

Defines Cloud Spanner Source API, and a naive implementation, similar to JdbcIO.

Also includes #3358

mairbek · 2017-06-19T20:39:55Z

R: @jkff

lukecwik · 2017-06-19T23:07:09Z

Retest this please

coveralls · 2017-06-20T01:05:35Z

Changes Unknown when pulling 9460ad0 on mairbek:readapi-pr into ** on apache:master**.

jkff

Thanks! Here's a first round.

jkff · 2017-06-20T21:51:32Z

...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/CreateTransactionFn.java

+/** Creates a batch transaction. */
+class CreateTransactionFn extends AbstractSpannerFn<Object, Transaction> {
+
+  private static final long serialVersionUID = -4174426331092286581L;


You don't need serialVersionUID on DoFn's

jkff · 2017-06-20T22:18:32Z

...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/CreateTransactionFn.java

+        databaseClient().readOnlyTransaction(config.getTimestampBound())) {
+      // Run a dummy sql statement to force the RPC and obtain the timestamp from the server.
+      ResultSet resultSet = readOnlyTransaction.executeQuery(Statement.of("SELECT 1"));
+      while (resultSet.next()) {


Is this necessary?

Yes, ResultSet#next actually send the create transaction gRCP request.

jkff · 2017-06-20T22:23:06Z

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

+    return builder().build();
+  }
+
+  public static Builder builder() {


You're already exposing the withBlah() methods - no need to expose the Builder class as public.

jkff · 2017-06-20T22:23:35Z

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

+  /** Builder for {@link SpannerConfig}. */
+  @AutoValue.Builder
+  public abstract static class Builder {
+


A lot of unnecessary blank lines

Cleaned this up

jkff · 2017-06-20T22:25:54Z

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

+    /**
+     * Returns a new {@link SpannerIO.Read} that will write to the specified Cloud Spanner project.
+     *
+     * <p>Does not modify this object.


Remove all the "Does not modify this object" comments - this is true for all withBlah() methods in Beam and specifying it is redundant.

You can also drop "Returns a new SpannerIO.Read that..." - usually we phrase these comments as simply "Specifies the Cloud Spanner project" (note: your comment currently says write - that's a copy-paste artifact I suppose; same is true for a bunch of other builder methods below)

jkff · 2017-06-20T22:31:11Z

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

+    }
+
+    /**
+     * Returns a new {@link SpannerIO.Read} that will read from the specified Cloud Spanner


This comment is incorrect

jkff · 2017-06-20T22:32:41Z

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

+  public abstract static class CreateTransaction
+      extends PTransform<PBegin, PCollectionView<Transaction>> {
+
+    private static final long serialVersionUID = 9201734106453817417L;


serialVersionUID's on PTransforms are also unnecessary. We don't serialize PTransform's using Serializable.

jkff · 2017-06-20T22:35:34Z

...oogle-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/SpannerIOReadTest.java

+/** Unit tests for {@link SpannerIO}. */
+@RunWith(JUnit4.class)
+public class SpannerIOReadTest implements Serializable {
+  @Rule public final transient TestPipeline pipeline = TestPipeline.create();


Seems like you're not using this pipeline. Were you planning to add some tests reading from a mock spanner instance?

jkff · 2017-06-20T22:38:56Z

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

+  /** Pipeline options for this test. */
+  public interface SpannerTestPipelineOptions extends TestPipelineOptions {
+    @Description("Project ID for Spanner")
+    @Default.String("apache-beam-testing")


Hmm, looks like this test can only be run by people who are members of the apache-beam-testing project (e.g. I'm not).

Better implement tests with mocks, like we do for BigQuery and many other connectors. If you want to also have ITs that's fine, but let's discuss it separately after the mocks.

This is a project that beam CI uses. @dhalperi set up the Spanner config.

Agree, I'm keeping this test and will add more tests to SpannerIOReadTest.

@davorbonaci or @jasonkuster can say more about the ITs. It is standard for the ITs to default to a project that Jenkins can access, but that is not public if the underlying service doesn't make it easy. (BigQuery's ReadITs can use public datasets, but not for writing... etc).

Humans can run these by overriding the arguments on the command line / in the test config.

mairbek · 2017-06-22T01:14:07Z

Added more tests, please take a look.

jkff

Thanks! Here's another round; starting to look pretty good.

jkff · 2017-06-23T01:47:41Z

...ogle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/NaiveSpannerReadFn.java

+        databaseClient().readOnlyTransaction(timestampBound)) {
+      ResultSet resultSet = execute(readOnlyTransaction);
+      while (resultSet.next()) {
+        c.output(resultSet.getCurrentRowAsStruct());


Btw, is this efficient? Or is it normally a lot more efficient for users to construct their own objects from the ResultSet directly, like people normally do with JDBC ResultSet's?

It should be reasonably efficient.

We might want to introduce a Schema abstraction to avoid serialization overhead (currently, we serialize pairs of name, value both for struct and mutation). Something for the future PR.

jkff · 2017-06-23T01:57:57Z

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

+    abstract SpannerConfig getSpannerConfig();
+
+    @Nullable
+    abstract Long getBatchSizeBytes();


Change it back to long? :)

Merged with master

jkff · 2017-06-23T01:58:52Z

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

-    Write withServiceFactory(ServiceFactory<Spanner, SpannerOptions> serviceFactory) {
-      return toBuilder().setServiceFactory(serviceFactory).build();
+    /** Specifies the batch size limit. */
+    public Write withBatchSizeBytes(long batchSizeBytes) {


Making this public should not be part of the current PR too, right?

It was public before. I've moved methods around.

jkff · 2017-06-23T01:59:44Z

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

-      }
+      getSpannerConfig().populateDisplayData(builder);
+      builder.add(
+          DisplayData.item("batchSizeBytes", getBatchSizeBytes()).withLabel("Batch Size in Bytes"));


Since it's not public and controllable by user, it should not be in display data

it's public.

jkff · 2017-06-23T02:00:12Z

...gle-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/spanner/SpannerWriteGroupFn.java

@@ -0,0 +1,125 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one


I welcome this move, but it should not be part of the current PR

Would you prefer if I send e9528e1 as a separate RP?

jkff · 2017-06-23T02:01:18Z

...ogle-cloud-platform/src/test/java/org/apache/beam/sdk/io/gcp/spanner/FakeServiceFactory.java

+import org.mockito.Matchers;
+
+/**
+ * A serialization friendly type service factory that maintains a mock {@link Spanner} and


Why does it need to be serializable?

It's part of the Transform which gets serialized by TestPipeline.

…rFn`

coveralls · 2017-06-28T01:32:57Z

Coverage decreased (-0.02%) to 70.869% when pulling 45dda68 on mairbek:readapi-pr into 2365e71 on apache:master.

jkff

Thanks, I'll merge soon.

jkff reviewed Jun 20, 2017

View reviewed changes

jkff requested changes Jun 23, 2017

View reviewed changes

Mairbek Khadikov added 14 commits June 27, 2017 14:37

Support ValueProviders.

560b003

ValueProvider for the batch size is not a good idea.

78537fd

Bump spanner version

9bde450

Pre read api refactoring. Extract SpannerConfig and `AbstractSpanne…

e9528e1

…rFn`

Read api with naive implementation

f3ee44b

Tests

3f273ac

License headers

29c35e5

Deps hell

13a280f

Removed serialVersionUID

f28af5b

make SpannerConfig.Builder package private.

23d8910

Vertical space

d3cc7e6

Reviewed documentation

1286935

Better test coverage.

2a8e0e5

Making checkstyle happy

47e7120

mairbek force-pushed the readapi-pr branch from 32123d9 to 47e7120 Compare June 27, 2017 21:38

Mairbek Khadikov added 2 commits June 27, 2017 15:39

Long -> long

237f020

happy mvn clean install

45dda68

jkff approved these changes Jun 28, 2017

View reviewed changes

asfgit closed this in e5929bd Jun 28, 2017

		@@ -0,0 +1,125 @@
		/*
		* Licensed to the Apache Software Foundation (ASF) under one

[BEAM-1542] Cloud Spanner Source #3395

[BEAM-1542] Cloud Spanner Source #3395

Conversation

mairbek commented Jun 19, 2017

mairbek commented Jun 19, 2017

lukecwik commented Jun 19, 2017

coveralls commented Jun 20, 2017

jkff 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

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 Jun 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mairbek commented Jun 22, 2017

jkff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mairbek Jun 27, 2017 • edited

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

coveralls commented Jun 28, 2017

jkff left a comment

Choose a reason for hiding this comment

dhalperi Jun 21, 2017 •

edited

mairbek Jun 27, 2017 •

edited