Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(samples): jsonstreamwriter samples #756

Merged
merged 21 commits into from
Jan 13, 2021

Conversation

VeronicaWasson
Copy link
Contributor

No description provided.

@VeronicaWasson VeronicaWasson requested a review from a team December 30, 2020 19:45
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Dec 30, 2020
@product-auto-label product-auto-label bot added api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. samples Issues that are directly related to samples. labels Dec 30, 2020
@codecov
Copy link

codecov bot commented Dec 30, 2020

Codecov Report

Merging #756 (edd650d) into master (0491bcb) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #756   +/-   ##
=========================================
  Coverage     80.89%   80.89%           
  Complexity      992      992           
=========================================
  Files            74       74           
  Lines          5333     5333           
  Branches        412      412           
=========================================
  Hits           4314     4314           
  Misses          847      847           
  Partials        172      172           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0491bcb...edd650d. Read the comment docs.

@stephaniewang526
Copy link
Contributor

stephaniewang526 commented Dec 30, 2020

please follow docs(samples): region_tag sample format for initial commit and PR subject.

Example: googleapis/java-bigquery@37c0632

Please make sure to correct your initial commit.

@VeronicaWasson
Copy link
Contributor Author

VeronicaWasson commented Jan 5, 2021

please follow docs(samples): region_tag sample format for initial commit and PR subject.

I added 'doc(samples)' - What should I use for region_tag and sample in this case?

@VeronicaWasson VeronicaWasson changed the title Code samples for JsonStreamWriter docs(samples) JsonStreamWriter sample Jan 5, 2021
package com.example.bigquerystorage;

import com.google.api.core.ApiFuture;
import com.google.cloud.bigquery.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

please import only used packages and avoid *

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

.build()) {

int offsets[] = {0, 1, 2, 3, 4, 5, 5};
// The last offset is repeated. This will cause an ALREADY_EXISTS error.
Copy link
Contributor

Choose a reason for hiding this comment

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

The convention is to place the comment above the statement. More importantly, what is an offset and what does it do? Why are we showing a repeated offset if it causes an error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

public class WriteCommittedStream {

public static Status.Code getStatusCode(StatusRuntimeException e) {
return e.getStatus().getCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

this helper method seems redundant -- we can directly write this one liner in the sample body.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this and simplified the exception handling.


System.out.println("Appended records successfully.");

} catch (Exception e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a more specific type of Exception we can catch here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are several - Is it better to catch each type explicitly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed - now I catch a specific type and declare others as thrown

* limitations under the License.
*/

package com.example.bigquerystorage;
Copy link
Contributor

Choose a reason for hiding this comment

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

One test class per sample class please.

@Test
public void testWriteCommittedStream() throws Exception {
WriteCommittedStream.writeCommittedStream(
GOOGLE_CLOUD_PROJECT, BIGQUERY_DATASET_NAME, BIGQUERY_TABLE_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

it does not look like we are testing anything here.
we need to use Truth to assert that we are getting desirable print output. Something like: https://github.com/googleapis/java-bigquery/blob/9945ca699eb7e439da387c40ac338a473dd9cb81/samples/snippets/src/test/java/com/example/bigquery/CopyMultipleTablesIT.java#L77

public void testWriteToDefaultStream() throws Exception {
WriteCommittedStream.writeToDefaultStream(
GOOGLE_CLOUD_PROJECT, BIGQUERY_DATASET_NAME, BIGQUERY_TABLE_NAME);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

see above comment and also this should be in a separate test class.

public void testWritePendingStream() throws Exception {
WritePendingStream.writePendingStream(
GOOGLE_CLOUD_PROJECT, BIGQUERY_DATASET_NAME, BIGQUERY_TABLE_NAME);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

see above comment and also this should be in a separate test class.

@stephaniewang526
Copy link
Contributor

stephaniewang526 commented Jan 5, 2021

please follow docs(samples): region_tag sample format for initial commit and PR subject.

I added 'doc(samples)' - What should I use for region_tag and sample in this case?

region_tag needs to be created in devrel samples tracker so that it can go on the documentation site. please let me know if you would like me to create them.

@stephaniewang526
Copy link
Contributor

stephaniewang526 commented Jan 5, 2021

@stephaniewang526
Copy link
Contributor

stephaniewang526 commented Jan 5, 2021

Also prior to raising a PR, please:

  1. run mvn verify locally in the project root to make sure the code is still compiling and all tests pass;
  2. run mvn -P lint --quiet --batch-mode checkstyle:check in the samples directory to make sure there is no checkstyle errors; and
  3. run mvn com.coveo:fmt-maven-plugin:format in the samples directory to reformat code.
    If everything passed, you are ready to raise a PR!

@snippet-bot
Copy link

snippet-bot bot commented Jan 8, 2021

Here is the summary of changes.

You added 3 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@VeronicaWasson VeronicaWasson changed the title docs(samples) JsonStreamWriter sample docs(samples): jsonstreamwriter samples Jan 8, 2021
@stephaniewang526 stephaniewang526 added the automerge Merge the pull request once unit tests and other checks pass. label Jan 8, 2021
@gcf-merge-on-green
Copy link

Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot.

VeronicaWasson and others added 21 commits January 12, 2021 01:51
Remove the backoff logic to keep the sample code simpler.
- Simplify code, remove duplicate-record example.
- Split all snippets and tests into separate classes.
- Add comments and javadocs links.
- Clean up imports.
- Add region tags.
- Catch only specific exceptions.
- Run linter and fmt-maven-plugin.
Create temporary dataset and table for sample integration tests
@stephaniewang526 stephaniewang526 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 12, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 12, 2021
@stephaniewang526
Copy link
Contributor

@yirutang please whitelist the devrel CI testing project

com.google.api.gax.rpc.PermissionDeniedException: io.grpc.StatusRuntimeException: PERMISSION_DENIED: Project 779844219229 doesn't have permission to access Write API

full trace

@stephaniewang526 stephaniewang526 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 12, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 12, 2021
@stephaniewang526 stephaniewang526 merged commit 929b2ce into googleapis:master Jan 13, 2021
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 14, 2021
🤖 I have created a release \*beep\* \*boop\* 
---
### [1.8.4](https://www.github.com/googleapis/java-bigquerystorage/compare/v1.8.3...v1.8.4) (2021-01-14)


### Bug Fixes

* default stream integration test failures due to production expected change ([#791](https://www.github.com/googleapis/java-bigquerystorage/issues/791)) ([1c2b5c1](https://www.github.com/googleapis/java-bigquerystorage/commit/1c2b5c1ef478305fe7f3d9f1843750cec18ba9f8))


### Documentation

* **samples:** jsonstreamwriter samples ([#756](https://www.github.com/googleapis/java-bigquerystorage/issues/756)) ([929b2ce](https://www.github.com/googleapis/java-bigquerystorage/commit/929b2cea1951bbe45eea596163f9a7a74d0ab041))


### Dependencies

* update dependency com.google.cloud:google-cloud-bigquery to v1.126.6 ([#794](https://www.github.com/googleapis/java-bigquerystorage/issues/794)) ([8e68546](https://www.github.com/googleapis/java-bigquerystorage/commit/8e68546f1e86553919766f9333ad911ba7da8442))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.18.0 ([#795](https://www.github.com/googleapis/java-bigquerystorage/issues/795)) ([86036bb](https://www.github.com/googleapis/java-bigquerystorage/commit/86036bb5caca125b38a64bd63acc5486a87b8e35))
* update protobuf ([#790](https://www.github.com/googleapis/java-bigquerystorage/issues/790)) ([792e925](https://www.github.com/googleapis/java-bigquerystorage/commit/792e925840e99033a1f194b2bfb372dae79d3d0d))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
shubhwip pushed a commit to shubhwip/java-bigquerystorage that referenced this pull request Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. cla: yes This human has signed the Contributor License Agreement. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants