docs(samples): add UpdateUserEventsJson implementation #388

sborisenkox · 2022-03-30T10:27:52Z

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.

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

Neenu1995 · 2022-03-30T21:30:37Z

samples/interactive-tutorials/src/main/java/events/setup/UpdateUserEventsJson.java

+    String invalidFilePath = "src/main/resources/user_events_some_invalid.json";
+
+    updateEventsTimestamp(filePath);
+    updateEventsTimestamp(invalidFilePath);


Does the test for these already exist?

There are no tests for classes which in 'setup' package.

These samples need tests added.

I really dislike this pattern of relying the main to test. It's inconsistent with how the other java samples are tested, and showing both a valid and an invalid example just makes the example more confusing.

Could you please clarify, what exactly issue with the tests you observe?
Currently, all the tests are relying on the pattern you recommended on past PR’s.
#303 (comment)

I did not recommend using the main for testing. In fact, I explictly recommended against it: #303 (comment)

Here's example of how it's typically handled (calling the snippet directly): https://github.com/GoogleCloudPlatform/java-docs-samples/blob/e1ec7d8800f83db0bc7dd3b6f02cfa634802ec52/kms/src/main/java/kms/CreateKeyHsm.java#L41

Do you mean to remove main method calls from unit tests? Just call methods directly?

@sborisenkox Yes.

This lets you test for different conditions individually (one test for filepath and one for invalidFilePath) and is less confusing for users to navigate.

These samples need tests added.

I really dislike this pattern of relying the main to test. It's inconsistent with how the other java samples are tested, and showing both a valid and an invalid example just makes the example more confusing.

Tests for the setup package were added.

Unit tests and some classes were refactored.

Please review.

kurtisvg · 2022-03-31T15:53:51Z

samples/interactive-tutorials/src/main/java/events/setup/UpdateUserEventsJson.java

+    String invalidFilePath = "src/main/resources/user_events_some_invalid.json";
+
+    updateEventsTimestamp(filePath);
+    updateEventsTimestamp(invalidFilePath);


These samples need tests added.

I really dislike this pattern of relying the main to test. It's inconsistent with how the other java samples are tested, and showing both a valid and an invalid example just makes the example more confusing.

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

kurtisvg

Please take some time to review the Sample Style Guide. And look at some example snippets. Here's a good example of a snippet following the style guide:

https://github.com/GoogleCloudPlatform/java-docs-samples/blob/main/kms/src/main/java/kms/CreateKeyAsymmetricDecrypt.java

kurtisvg · 2022-04-08T15:46:17Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

+    String bucketName = System.getenv("EVENTS_BUCKET_NAME");
+    String gcsBucket = String.format("gs://%s", bucketName);
+    String gcsErrorsBucket = String.format("%s/error", gcsBucket);
+    String gcsUserEventsObject = "user_events.json";
+    // TO CHECK ERROR HANDLING USE THE JSON WITH INVALID USER EVENT:
+    // gcsEventsObject = "user_events_some_invalid.json"
+
+    ImportUserEventsRequest importEventsGcsRequest =
+        getImportEventsGcsRequest(gcsUserEventsObject, gcsBucket, gcsErrorsBucket, defaultCatalog);
+    importUserEventsFromGcs(importEventsGcsRequest);
+  }


The main function should be example values only. See Method Structure in the Samples Style Guide.

It should look something like:

public static void main(String[] args) { // TODO(developer): Replace these variables before running the sample. String catalog = String.format( "projects/%s/locations/global/catalogs/%s", "your-project-id" "your-catalog-name") // Path to a file containing UserEvents to import in a GCS bucket String gcsFile = String.format("gs://your-bucket-name/path/to/file.json") importUserEventsFromGcs(catalog, gcsFile); }

kurtisvg · 2022-04-08T15:49:44Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

+
+      while (!operation.getDone()) {
+        System.out.println("Please wait till operation is done.");
+        Thread.sleep(30_000);


Please use the java.time package here.

Suggested change

Thread.sleep(30_000);

TimeUnit.SECONDS.sleep(30);

kurtisvg · 2022-04-08T15:50:12Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

+      OperationsClient operationsClient = serviceClient.getOperationsClient();
+      Operation operation = operationsClient.getOperation(operationName);
+
+      while (!operation.getDone()) {


I would recommend adding a timeout of some form to this loop so it cannot continue indefinitely.

kurtisvg · 2022-04-08T15:51:29Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

-          System.out.printf("Operation result: %s%n", response);
-        }
-      } catch (InvalidArgumentException e) {
+    try (UserEventServiceClient serviceClient = UserEventServiceClient.create()) {


Make sure to include the comment regarding client best practices.

kurtisvg · 2022-04-08T15:52:27Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

+  public static ImportUserEventsRequest getImportEventsGcsRequest(
+      String gcsEventsObject, String gcsBucket, String gcsErrorsBucket, String defaultCatalog) {
+    GcsSource gcsSource =
+        GcsSource.newBuilder()
+            .addInputUris(String.format("%s/%s", gcsBucket, gcsEventsObject))
+            .build();
+
+    UserEventInputConfig inputConfig =
+        UserEventInputConfig.newBuilder().setGcsSource(gcsSource).build();
+
+    System.out.println("GRS source: " + gcsSource.getInputUrisList());

-    importUserEventsFromGcs(gcsEventsObject, defaultCatalog);
+    ImportErrorsConfig errorsConfig =
+        ImportErrorsConfig.newBuilder().setGcsPrefix(gcsErrorsBucket).build();
+
+    ImportUserEventsRequest importRequest =
+        ImportUserEventsRequest.newBuilder()
+            .setParent(defaultCatalog)
+            .setInputConfig(inputConfig)
+            .setErrorsConfig(errorsConfig)
+            .build();
+
+    System.out.printf("Import user events from google cloud source request: %s%n", importRequest);
+
+    return importRequest;


This doesn't need to be a seperate method - we should only be adding additional methods if it improve readability. See Class Structure.

I've changed some classes, but others should have methods calls to prepare snippet resources.

kurtisvg · 2022-04-08T15:57:26Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

      }
-    } catch (BigQueryException e) {
-      System.out.printf("Exception message: %s", e.getMessage());
+    } catch (InvalidArgumentException e) {


When can this "InvalidArgumentException" happen? Can we add a try/catch for this exception just where it's relevant?

It's relevant in that case when bucket has no specified file.
Also added PermissionDeniedException for case when env variable bucketName can't be read properly.

kurtisvg · 2022-04-08T15:58:27Z

samples/interactive-tutorials/src/main/java/events/setup/EventsCreateGcsBucket.java

+    Timestamp currentDate =
+        Timestamp.newBuilder()
+            .setSeconds(Instant.now().getEpochSecond())
+            .setNanos(Instant.now().getNano())
+            .build();


This doesn't need to be in main (it should be part of the sample).

We don't need to create a new bucket every time when running the unit test.
In the code sample, bucket name contains timestamp.
In the test class, bucket has static name.

kurtisvg · 2022-04-08T16:01:22Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

+          "Given GCS input path was not found. %n%s%n"
+              + "Please run CreateTestResources class to create resources.",


User's typically copy/paste these samples to run. If they need a specific resource, we should explain to them how to make it in a comment at the top of the snippet.

Suggested change

"Given GCS input path was not found. %n%s%n"

+ "Please run CreateTestResources class to create resources.",

"The specified GCS file does not exist. Please make sure to create one following the instructions at the top of this file. ",

Explanation and all setup instructions will be described in a separate space.
Comment was changed.

kurtisvg · 2022-04-08T16:08:56Z

samples/interactive-tutorials/src/main/java/events/setup/EventsCreateGcsBucket.java

-
-  private static final String BUCKET_NAME =
-      String.format("%s_events_%s", PROJECT_ID, CURRENT_DATE.getSeconds());
-
  public static void main(String... args) throws IOException {


these have to be String[] to be proper main functions.

kurtisvg · 2022-04-08T16:10:55Z

samples/interactive-tutorials/src/main/java/events/ImportUserEventsGcs.java

+    // TO CHECK ERROR HANDLING USE THE JSON WITH INVALID USER EVENT:
+    // gcsEventsObject = "user_events_some_invalid.json"


This should just be a separate test in your tests file. You can have one where you call a valid file (e.g.importUimportUserEventsFromGcs("your-catalog", "user-events.json) and one where you call an invalid file (e.g.importUimportUserEventsFromGcs("your-catalog", "some-invalid-file.json). You shouldn't be depending on someone to edit a comment out, because that means it's not being run the in the CI.

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

kurtisvg

Can you please refactor this into smaller CLs? There are 68 changed files here and a diff of +- 2,000 lines. This is way too much to review in a single CL.

sborisenkox · 2022-04-25T13:16:55Z

Can you please refactor this into smaller CLs? There are 68 changed files here and a diff of +- 2,000 lines. This is way too much to review in a single CL.

I have split into 4 PR's.
#415
#417
#418
#419

I close this PR.

tetiana-karasova and others added 3 commits March 22, 2022 01:34

Update README

b998aa2

Merge branch 'googleapis:main' into main

4bedf81

Add UpdateUserEventsJson impl.

68c44f0

sborisenkox requested review from a team as code owners March 30, 2022 10:27

product-auto-label bot added the api: retail Issues related to the googleapis/java-retail API. label Mar 30, 2022

kweinmeister added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 30, 2022

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

kweinmeister added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 30, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 30, 2022

🦉 Updates from OwlBot post-processor

c6cafa2

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

chenlei1216 approved these changes Mar 30, 2022

View reviewed changes

Merge branch 'googleapis:main' into main

213a52f

Neenu1995 closed this Mar 30, 2022

Neenu1995 reopened this Mar 30, 2022

Neenu1995 reviewed Mar 30, 2022

View reviewed changes

kweinmeister changed the title ~~Add UpdateUserEventsJson impl.~~ docs(samples): add UpdateUserEventsJson implementation Mar 30, 2022

sborisenkox requested a review from Neenu1995 March 31, 2022 07:32

product-auto-label bot added the samples Issues that are directly related to samples. label Mar 31, 2022

blunderbuss-gcf bot assigned kurtisvg Mar 31, 2022

the bash scripts are added

756a39c

kurtisvg suggested changes Mar 31, 2022

View reviewed changes

sborisenkox and others added 5 commits March 31, 2022 19:15

Merge branch 'googleapis:main' into update-events-json

47cf534

Update README.md

8bd8e92

🦉 Updates from OwlBot post-processor

108a637

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

Update user_environment_setup.sh

5f976e4

Update user_import_data_to_catalog.sh

475ef6f

sborisenkox requested a review from kurtisvg April 4, 2022 15:16

Update README.md

1df9f7a

sborisenkox added 2 commits April 5, 2022 16:14

Add unit tests for setup package.

8ee5596

Refactoring.

d759ca0

Neenu1995 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 5, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 5, 2022

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

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

gcf-owl-bot bot and others added 2 commits April 5, 2022 17:28

🦉 Updates from OwlBot post-processor

54b8798

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

Merge branch 'main' into main

23b8ff4

Neenu1995 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 6, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 6, 2022

sborisenkox and others added 2 commits April 7, 2022 16:16

Merge branch 'googleapis:main' into main

8fffe9e

Merge branch 'main' into update-events-json

923cfb2

kurtisvg suggested changes Apr 8, 2022

View reviewed changes

sborisenkox added 2 commits April 12, 2022 17:40

Remove outdated comment.

bb2dc2a

Refactoring.

3ea57ae

product-auto-label bot added the size: xl Pull request size is extra large. label Apr 14, 2022

sborisenkox added 2 commits April 18, 2022 17:19

Add unit tests.

611aadf

Refactoring.

74f9c75

sborisenkox requested a review from kurtisvg April 18, 2022 14:21

kweinmeister added kokoro:force-run Add this label to force Kokoro to re-run the tests. owlbot:run Add this label to trigger the Owlbot post processor. labels Apr 20, 2022

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

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Apr 20, 2022

🦉 Updates from OwlBot post-processor

df6c157

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

kurtisvg suggested changes Apr 22, 2022

View reviewed changes

sborisenkox closed this Apr 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs(samples): add UpdateUserEventsJson implementation #388

docs(samples): add UpdateUserEventsJson implementation #388

sborisenkox commented Mar 30, 2022

Neenu1995 Mar 30, 2022 •

edited

Loading

sborisenkox Mar 31, 2022

kurtisvg Mar 31, 2022

sborisenkox Apr 1, 2022 •

edited

Loading

kurtisvg Apr 4, 2022

sborisenkox Apr 4, 2022

kurtisvg Apr 4, 2022

sborisenkox Apr 5, 2022 •

edited

Loading

kurtisvg Mar 31, 2022

kurtisvg left a comment

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 14, 2022

kurtisvg Apr 8, 2022

sborisenkox Apr 18, 2022

kurtisvg left a comment

sborisenkox commented Apr 25, 2022 •

edited

Loading

		"Given GCS input path was not found. %n%s%n"
		+ "Please run CreateTestResources class to create resources.",

	"Given GCS input path was not found. %n%s%n"
	+ "Please run CreateTestResources class to create resources.",
	"The specified GCS file does not exist. Please make sure to create one following the instructions at the top of this file. ",

		// TO CHECK ERROR HANDLING USE THE JSON WITH INVALID USER EVENT:
		// gcsEventsObject = "user_events_some_invalid.json"

docs(samples): add UpdateUserEventsJson implementation #388

docs(samples): add UpdateUserEventsJson implementation #388

Conversation

sborisenkox commented Mar 30, 2022

Neenu1995 Mar 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sborisenkox Apr 1, 2022 • edited Loading

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

sborisenkox Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtisvg 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

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

kurtisvg left a comment

Choose a reason for hiding this comment

sborisenkox commented Apr 25, 2022 • edited Loading

Neenu1995 Mar 30, 2022 •

edited

Loading

sborisenkox Apr 1, 2022 •

edited

Loading

sborisenkox Apr 5, 2022 •

edited

Loading

sborisenkox commented Apr 25, 2022 •

edited

Loading