doc(samples): rewrite PendingStream Write API sample to match best practices by gnanda · Pull Request #1603 · googleapis/java-bigquerystorage

gnanda · 2022-03-31T23:58:08Z

split samples into initialization, append, cleanup
Change appends to async

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
Ensure the tests and linter pass
Code coverage does not decrease (if any source code was changed)
Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

stephaniewang526 · 2022-04-01T14:57:19Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteToDefaultStream.java

-    try (JsonStreamWriter writer =
-        JsonStreamWriter.newBuilder(parentTable.toString(), tableSchema).build()) {
+    try {
      // Write two batches to the stream, each with 10 JSON records. A writer should be used for as


Can we add some explanation here when to use "batches"? My question reading this sample is why not just write 20 JSON records using 1 batch (why 2 batches)?

Added a comment.

stephaniewang526 · 2022-04-01T14:59:10Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteToDefaultStream.java

+
+  public static WriteToDefaultStream create(String projectId, String datasetName, String tableName)
+      throws DescriptorValidationException, IOException, InterruptedException {
+    BigQuery bigquery = BigQueryOptions.getDefaultInstance().getService();


Can we add a comment here to explain why we are bringing in the BQ client and around schema conversion?

Will do in followup; I changed this to just be PendingStream which encompasses the other cases, so we can finalize on a design in one place and replicate to the others.

stephaniewang526 · 2022-04-01T15:06:07Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteToDefaultStream.java


 public class WriteToDefaultStream {

+  private final Phaser inflightRequestPhaser = new Phaser(1);


We don't prefer having anything out here like this, they should go inside either the driver method (runWriteToDefaultStream) or the body of the sample (writeToDefaultStream). Also, we need comment (explanation) around why we are using inflightRequestPhaser.

Added a comment about the Phaser (which we need in order to wait for all responses before closing the stream)

Discussed the variables offline: the Write API is a special case where we need a stateful client and want to make explicit that users should keep the writer around for as long as possible.

stephaniewang526 · 2022-04-01T15:25:55Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteToDefaultStream.java

+      throws DescriptorValidationException, IOException, ExecutionException {
+    ApiFuture<AppendRowsResponse> future = this.streamWriter.append(jsonArr);
+    this.inflightRequestPhaser.register();
+    ApiFutures.addCallback(


Can we add a comment here to explain this?

yirutang · 2022-04-01T18:30:04Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteCommittedStream.java

 import org.json.JSONArray;
 import org.json.JSONObject;

 public class WriteCommittedStream {


This name is a little confusing, maybe name it to WriteCommittedStreamSample? I am not sure the impact of changing the sample's name.

I think most of our samples don't include "Sample" in the name. e.g., createTable.

Maybe WriteToCommittedStream?

Make sense. Currently it sounded like a Stream. Will we just create a new file, switch the pointer and delete the old file?

sgtm I'll do this in a followup

yirutang · 2022-04-01T18:35:13Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteCommittedStream.java

+
+  public void cleanup() {
+    // Wait for all in-flight requests to complete.
+    this.inflightRequestPhaser.arriveAndAwaitAdvance();


Close the JsonWriter here? Since you are doing a Finalize, with close, maybe we don't need to wait for inflight ourselves?

Added the close, but as we discussed offline, we still need to wait for inflight requests to finish

VeronicaWasson · 2022-04-01T18:48:10Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteCommittedStream.java


  public static void writeCommittedStream(String projectId, String datasetName, String tableName)
      throws DescriptorValidationException, InterruptedException, IOException {
+    // One time initialization.


Can we put the initialization code into a separate writer.initialize() method? I really like the structure of #1585 from a docs perspective.

Changed it back. Not sure how to do it in a way that both exemplifies the structure and is "Good Java"

yirutang · 2022-04-01T19:45:33Z

samples/snippets/src/main/java/com/example/bigquerystorage/WritePendingStream.java

+      if (response.hasError()) {
+        System.out.format("Error: %s\n", response.getError());
+      } else {
+        System.out.format("Append %d success\n", response.getAppendResult().getOffset().getValue());


For pending stream, show user a way to set stream_status_ and append to check on the status to fail the append?

Done, thought I'm not great at Java, so maybe there's a better way?

yirutang · 2022-04-01T19:46:13Z

samples/snippets/src/main/java/com/example/bigquerystorage/WritePendingStream.java

+
+    // Once all streams are done, commit all of them in one request. This example only has the one
+    // stream.
+    BatchCommitWriteStreamsRequest commitRequest =


We shouldn't commit for failed writes?

Added an exception throw in cleanup and a comment about only committing successful streams.

yirutang · 2022-04-01T19:46:43Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteToDefaultStream.java

+
+    public void onSuccess(AppendRowsResponse response) {
+      if (response.hasError()) {
+        System.out.format("Error: %s\n", response.getError());


show a simple retry here?

yirutang · 2022-04-01T19:48:06Z

samples/snippets/src/main/java/com/example/bigquerystorage/WriteCommittedStream.java

+    }
+
+    public void onSuccess(AppendRowsResponse response) {
+      if (response.hasError()) {


I don't know what to do here...Maybe add a comment of how this can be retried? If we don't have a good retry case, should we have this sample?

My $0.02: I think it's better to show how to check the AppendRowsResponse error status, rather than not show anything here.

IIUC StreamWriter already handles this field which will invoke onFailure instead

java-bigquerystorage/google-cloud-bigquerystorage/src/main/java/com/google/cloud/bigquery/storage/v1/StreamWriter.java

Line 565 in 54874be

if (response.hasError()) {

I updated that code to show a little error handling; not sure if there's something more specific to show there

Start with just Pending stream which is the most complex to finalize design. Will update the other two in followup once consensus is reached.

gnanda · 2022-04-05T23:03:49Z

Updated based on comments, and changed it to be only WritePendingStream for now since it encompasses the other cases. Once we finalize on a plan, I'll update the other 2 to match the same structure

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

gcf-merge-on-green · 2022-04-10T03:36:36Z

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.

gnanda requested review from stephaniewang526 and yirutang March 31, 2022 23:58

product-auto-label bot added api: bigquerystorage Issues related to the googleapis/java-bigquerystorage API. samples Issues that are directly related to samples. labels Mar 31, 2022

blunderbuss-gcf bot assigned Shabirmean Mar 31, 2022

gnanda force-pushed the samples branch from 155e219 to dd0b7cb Compare April 1, 2022 00:14

stephaniewang526 changed the title ~~feat: Rewrite Write API samples to match best practices~~ doc(samples): rewrite Write API samples to match best practices Apr 1, 2022

stephaniewang526 added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 1, 2022

stephaniewang526 unassigned Shabirmean Apr 1, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 1, 2022

stephaniewang526 self-assigned this Apr 1, 2022

stephaniewang526 suggested changes Apr 1, 2022

View reviewed changes

yirutang reviewed Apr 1, 2022

View reviewed changes

VeronicaWasson reviewed Apr 1, 2022

View reviewed changes

yirutang reviewed Apr 1, 2022

View reviewed changes

dark0dave mentioned this pull request Apr 2, 2022

JsonToProtoMessage issue with TIMESTAMP fields - milliseconds vs microseconds #1604

Closed

feat: Rewrite Write API samples to match best practices

75c362a

Start with just Pending stream which is the most complex to finalize design. Will update the other two in followup once consensus is reached.

gnanda force-pushed the samples branch from 6160195 to 75c362a Compare April 5, 2022 22:25

gnanda changed the title ~~doc(samples): rewrite Write API samples to match best practices~~ doc(samples): rewrite PendingStream Write API sample to match best practices Apr 5, 2022

yirutang approved these changes Apr 6, 2022

View reviewed changes

gnanda requested a review from stephaniewang526 April 8, 2022 20:43

stephaniewang526 approved these changes Apr 9, 2022

View reviewed changes

stephaniewang526 added the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 9, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 9, 2022

stephaniewang526 added automerge Merge the pull request once unit tests and other checks pass. owlbot:run Add this label to trigger the Owlbot post processor. labels Apr 9, 2022

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Apr 9, 2022

🦉 Updates from OwlBot post-processor

5f8c384

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

gnanda requested a review from a team April 9, 2022 21:34

product-auto-label bot added the size: m Pull request size is medium. label Apr 9, 2022

gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 10, 2022

stephaniewang526 merged commit 4145ae0 into googleapis:main Apr 11, 2022


		public class WriteToDefaultStream {

		private final Phaser inflightRequestPhaser = new Phaser(1);

Conversation

gnanda commented Mar 31, 2022

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!

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!

yirutang Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VeronicaWasson Apr 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnanda commented Apr 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gcf-merge-on-green bot commented Apr 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yirutang Apr 1, 2022 •

edited

Loading

VeronicaWasson Apr 1, 2022 •

edited

Loading

gnanda commented Apr 5, 2022 •

edited

Loading