Updated Spanner Dataflow connector samples by mairbek · Pull Request #1059 · GoogleCloudPlatform/java-docs-samples

mairbek · 2018-03-13T22:52:54Z

No description provided.

lesv · 2018-03-20T02:08:16Z

- testing dataflow/spanner-io
------------------------------------------------------------
[ERROR] src/main/java/com/example/dataflow/SpannerGroupWrite.java:[16] (whitespace) EmptyLineSeparator: 'package' should be separated from previous statement.
[ERROR] src/main/java/com/example/dataflow/SpannerGroupWrite.java:[26] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - org.apache.beam.sdk.options.*.
[ERROR] src/main/java/com/example/dataflow/SpannerGroupWrite.java:[32] (sizes) LineLength: Line is longer than 100 characters (found 103).
[ERROR] src/main/java/com/example/dataflow/SpannerGroupWrite.java:[56] (whitespace) EmptyLineSeparator: 'METHOD_DEF' should be separated from previous statement.
[ERROR] src/main/java/com/example/dataflow/SpannerGroupWrite.java:[70] (sizes) LineLength: Line is longer than 100 characters (found 128).
[ERROR] src/main/java/com/example/dataflow/TransactionalRead.java:[29] (imports) AvoidStarImport: Using the '.*' form of import should be avoided - org.apache.beam.sdk.options.*.
[ERROR] src/main/java/com/example/dataflow/EstimateSize.java:[16] (whitespace) EmptyLineSeparator: 'package' should be separated from previous statement.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:2.17:check (default) on project dataflow-spanner: You have 7 Checkstyle violations. -> [Help 1]

[ID: 8307783] Build finished after 113 secs, exit value: 1


Warning: Permanently added 'localhost' (ECDSA) to the list of known hosts.
Build script failed with exit code: 1

mairbek · 2018-03-20T22:32:28Z

I've fixed checkstyle issues.

lesv

There aren't any tags for publishing.

lesv

Just nits, missing tests, maybe missing datafiles. Would be nice if there was a README.md explaining what's going on and how to use it.

I'm also asking for a better comment in the first file.

lesv · 2018-03-20T23:30:58Z

dataflow/spanner-io/src/main/java/com/example/dataflow/EstimateSize.java

+  }
+
+  /**
+   * Estimates the size of a Spanner row. For simplicity, arrays and structs aren't supported.


why do you do this?

What do you mean? There are two examples that estimate the logical size of the database.

I'm asking if the comment should explain why this is being done.

(ie. why do I need to do this?)

lesv · 2018-03-20T23:32:47Z

dataflow/spanner-io/src/main/java/com/example/dataflow/SpannerGroupWrite.java

+    void setDatabaseId(String value);
+
+    @Description("Singers output filename in the format: singer_id\tfirst_name\tlast_name")
+    @Default.String("data/usersids.txt")


Ok, but wouldn't putting it in resources be more appropriate?

Removed defaults.

…mple

mairbek · 2018-03-21T16:37:15Z

Please take a look, I've updated the tests and checked that the checkstyle passes

lesv · 2018-03-21T18:29:41Z

dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerGroupWriteIT.java

+    DatabaseClient dbClient = getDbClient();
+    try (ReadContext context = dbClient.singleUse()) {
+      ResultSet rs = context.executeQuery(
+          Statement.newBuilder("SELECT COUNT(*) FROM users WHERE STATE = @state").bind("state")


?? Why do you bother to bind to a constant? ??

(I'm ok w/ it as long as you had a reason)

It's just a good practice, if we want to change this to a variable in future

lesv · 2018-03-21T18:30:05Z

dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerGroupWriteIT.java

+    }
+    try (ReadContext context = dbClient.singleUse()) {
+      ResultSet rs = context.executeQuery(
+          Statement.newBuilder("SELECT COUNT(*) FROM PendingReviews WHERE ACTION = @action")


ditto, why bind to a constant?

It's just a good practice.

lesv · 2018-03-21T18:30:19Z

dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerGroupWriteIT.java

+        .getDatabaseClient(DatabaseId.of(spannerOptions.getProjectId(), instanceId, databaseId));
+  }
+
+}


Should there be a newLine here?

Added a new line

lesv · 2018-03-21T18:32:09Z

dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerWriteIT.java

+        .getDatabaseClient(DatabaseId.of(spannerOptions.getProjectId(), instanceId, databaseId));
+  }
+
+}


Added a new line

lesv

A few questions that may require changes, but I'm basically happy.

lesv · 2018-03-21T20:26:21Z

dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerReadIT.java

+        .getDatabaseClient(DatabaseId.of(spannerOptions.getProjectId(), instanceId, databaseId));
+  }
+
+}


I think your IDE is helping too much.

feel free to just merge once kokoro finishes.

…1.2 (#1059) [![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.cloud:libraries-bom](https://togithub.com/GoogleCloudPlatform/cloud-opensource-java) | `24.1.1` -> `24.1.2` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/24.1.2/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/24.1.2/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/24.1.2/compatibility-slim/24.1.1)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/24.1.2/confidence-slim/24.1.1)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-asset).

Updated Spanner Dataflow connector samples

bc04758

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 13, 2018

jsimonweb requested a review from davidcavazos March 14, 2018 00:25

Make checkstyle happy

2b40d68

jsimonweb requested review from lesv and removed request for davidcavazos March 20, 2018 22:49

Merge branch 'master' into sample

bca157e

lesv reviewed Mar 20, 2018

View reviewed changes

lesv previously approved these changes Mar 20, 2018

View reviewed changes

Mairbek Khadikov added 4 commits March 20, 2018 16:52

First test

fba6faa

Updated the test

0a8d691

Added tests

d89c0fb

Merge branch 'sample' of github.com:mairbek/java-docs-samples into sa…

91f007b

…mple

mairbek dismissed lesv’s stale review via 91f007b March 21, 2018 01:29

Mairbek Khadikov added 2 commits March 21, 2018 09:34

Makes checkstyle happy

7bad2e1

Use system properties

08c2629

Add add system property

419500f

lesv reviewed Mar 21, 2018

View reviewed changes

lesv previously approved these changes Mar 21, 2018

View reviewed changes

New lines

56b767e

mairbek dismissed lesv’s stale review via 56b767e March 21, 2018 18:48

Merge branch 'master' into sample

b2f1e55

lesv reviewed Mar 21, 2018

View reviewed changes

lesv approved these changes Mar 21, 2018

View reviewed changes

jsimonweb merged commit df10b39 into GoogleCloudPlatform:master Mar 21, 2018

Conversation

mairbek commented Mar 13, 2018

Uh oh!

lesv commented Mar 20, 2018

Uh oh!

mairbek commented Mar 20, 2018

Uh oh!

lesv left a comment

Choose a reason for hiding this comment

Uh oh!

lesv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mairbek commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lesv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants