FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

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

/ java-docs-samples Public

Updated Spanner Dataflow connector samples - #1059

Merged
jsimonweb merged 12 commits into
GoogleCloudPlatform:masterfrom
mairbek:sample
Mar 21, 2018
ConversationCommits (12)ChecksFiles changed
Merged

Updated Spanner Dataflow connector samples#1059
jsimonweb merged 12 commits into
GoogleCloudPlatform:masterfrom
mairbek:sample

Conversation

mairbek commented Mar 13, 2018

Copy link
Copy Markdown
Contributor

No description provided.

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

lesv commented Mar 20, 2018

Copy link
Copy Markdown
Contributor 
- 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 commented Mar 20, 2018

Copy link
Copy Markdown
Contributor Author

I've fixed checkstyle issues.

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

lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There aren't any tags for publishing.

lesv previously approved these changes Mar 20, 2018
View reviewed changes

lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread 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.

lesv Mar 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

why do you do this?

mairbek Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread dataflow/spanner-io/src/main/java/com/example/dataflow/SpannerGroupWrite.java Outdated
void setDatabaseId(String value);

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

lesv Mar 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

mairbek Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Removed defaults.

mairbek commented Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

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

lesv reviewed Mar 21, 2018
View reviewed changes
Comment thread 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")

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

mairbek Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lesv reviewed Mar 21, 2018
View reviewed changes
Comment thread 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")

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

ditto, why bind to a constant?

mairbek Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It's just a good practice.

lesv reviewed Mar 21, 2018
View reviewed changes
Comment thread dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerGroupWriteIT.java Outdated
.getDatabaseClient(DatabaseId.of(spannerOptions.getProjectId(), instanceId, databaseId));
}

} No newline at end of file

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should there be a newLine here?

mairbek Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added a new line

lesv reviewed Mar 21, 2018
View reviewed changes
Comment thread dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerWriteIT.java Outdated
.getDatabaseClient(DatabaseId.of(spannerOptions.getProjectId(), instanceId, databaseId));
}

} No newline at end of file

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

newLine?

mairbek Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added a new line

lesv previously approved these changes Mar 21, 2018
View reviewed changes

lesv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

lesv reviewed Mar 21, 2018
View reviewed changes
Comment thread dataflow/spanner-io/src/test/java/com/example/dataflow/SpannerReadIT.java
.getDatabaseClient(DatabaseId.of(spannerOptions.getProjectId(), instanceId, databaseId));
}

} No newline at end of file

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think your IDE is helping too much.

lesv Mar 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

feel free to just merge once kokoro finishes.

jsimonweb merged commit df10b39 into GoogleCloudPlatform:master Mar 21, 2018
minherz pushed a commit that referenced this pull request Nov 16, 2022
chore(deps): update dependency com.google.cloud:libraries-bom to v24.…
a736564 
…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.

---

 - [ ] <!-- rebase-check -->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).
minherz pushed a commit that referenced this pull request Nov 17, 2022
chore(deps): update dependency com.google.cloud:libraries-bom to v24.…
a62b024 
…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.

---

 - [ ] <!-- rebase-check -->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).
anguillanneuf pushed a commit that referenced this pull request Dec 5, 2022
chore(deps): update dependency com.google.cloud:libraries-bom to v24.…
23046ee 
…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.

---

 - [ ] <!-- rebase-check -->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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
lesv lesv approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL