feat: Add GCF Java Tips #1550

grant · 2019-08-16T01:04:06Z

Adds GCF Tips snippets:
https://cloud.google.com/functions/docs/bestpractices/tips

grant · 2019-08-16T02:50:37Z

Looks like I need to fix some linting issues:

[ERROR] src/main/java/Tips.java:[42] (indentation) Indentation: 'method def' child has incorrect indentation level 6, expected level should be 4.
[ERROR] src/main/java/Tips.java:[43] (indentation) Indentation: 'method def' child has incorrect indentation level 6, expected level should be 4.
[ERROR] src/main/java/Tips.java:[47] (indentation) Indentation: 'method def' child has incorrect indentation level 6, expected level should be 4.
[ERROR] src/main/java/Tips.java:[48] (indentation) Indentation: 'method def' child has incorrect indentation level 6, expected level should be 4.
[ERROR] src/main/java/Tips.java:[63,15] (naming) MethodName: Method name 'ScopesDemo' must match pattern '^[a-z][a-z0-9][a-zA-Z0-9_]*$'.
[ERROR] src/main/java/Tips.java:[74,15] (naming) MethodName: Method name 'TipsRetry' must match pattern '^[a-z][a-z0-9][a-zA-Z0-9_]*$'.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check (default) on project functions-snippets: You have 6 Checkstyle violations. -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

kurtisvg · 2019-08-16T15:11:11Z

functions/snippets/src/main/java/Tips.java

+  // [START run_tips_global_scope]
+  // Global (instance-wide) scope
+  // This computation runs at instance cold-start
+  public static final int INSTANCE_VAR = heavyComputation();


Class variables should be declared at the top of the class.

Also, this might not be the way we want to showcase this. Normally, global variables aren't used with servlets due to issues with thread-safety. This example is probably okay because it's a final variable, but we probably either want to add a warning or (better yet) update the example to us a ServletContextListener for creating start up resources.

Moved to the very top. Not sure if we need the context listener.

It should be updated to either use a ContextListener, or include a warning that global values in Servlets are not threadsafe, and a ContextListener is preferred for mutable variables.

I'm not really sure if/how a ContextListener would work with functions, so this snippet might be a good example to showcase that pattern as well.

I don't really know how to proceed. Do you think that GCF will use multiple threads per Function invocation?

This example is probably okay because it's a final variable

Since this is a final variable, isn't it safe to access across multiple threads?

Most servlet environments will use multiple threads to handle requests, so

Do you think that GCF will use multiple threads per Function invocation?

I don't know how GCF handles things, but it would be idiomatic to Java to remain threadsafe when using Servlets since other runtimes may use multiple threads.

Since this is a final variable, isn't it safe to access across multiple threads?

I should have been more clear - since this is a final and a primitive it's immutable, and thus thread-safe. However, we should either point out why this is okay, or show the user how to handle these properly. If the user tried to do this with an object that isn't thread-safe (even if it's final), they'll run into issues.

functions/snippets/src/main/java/Tips.java

kurtisvg · 2019-08-16T15:13:27Z

functions/snippets/src/main/java/Tips.java

+  private static HttpClient client =
+      HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(10)).build();
+
+  // [END functions_tips_scopes]


Excluding these functions breaks one of the goals of the new sample format, in that the sample isn't copy-paste-runnable. Can you leave them in (but maybe move them towards the bottom of the class since they aren't important?)

Sure.
I was just following the other samples that exclude them.
Can include them at the bottom.

kurtisvg · 2019-08-16T15:13:55Z

functions/snippets/src/main/java/Tips.java

+      return numbers.stream().reduce((t, x) -> t + x).get();
+  }
+
+  private static int heavyComputation() {


These functions are exactly the same - is this intended?

functions/snippets/src/main/java/Tips.java

kurtisvg · 2019-08-16T15:32:54Z

functions/snippets/src/main/java/Tips.java

+  // [END run_tips_global_scope]
+  // [END functions_tips_scopes]
+
+  public void TipsRetry(HttpServletRequest request, HttpServletResponse response)


I'm not sure what this function is doing - it should be in a different file if it's for a different snippet

kurtisvg · 2019-08-16T15:34:48Z

functions/snippets/src/main/java/Tips.java

+    HttpRequest getRequest = HttpRequest.newBuilder().uri(URI.create(url)).GET().build();
+
+    // Send the TipsRetry using the client
+    HttpResponse<String> getResponse = client.send(getRequest, BodyHandlers.ofString());


What does TipsRetry mean? This seems like just a simple get request?

grant

This PR should be good to go.
Adds this 1 snippet:
https://cloud.google.com/functions/docs/bestpractices/tips#use_dependencies_wisely

grant · 2019-08-23T22:12:16Z

functions/snippets/src/main/java/Tips.java

+  private static HttpClient client =
+      HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(10)).build();
+
+  // [END functions_tips_scopes]


Sure.
I was just following the other samples that exclude them.
Can include them at the bottom.

grant · 2019-08-23T22:12:38Z

functions/snippets/src/main/java/Tips.java

+      return numbers.stream().reduce((t, x) -> t + x).get();
+  }
+
+  private static int heavyComputation() {


grant · 2019-08-23T22:14:54Z

functions/snippets/src/main/java/Tips.java

+  // [START run_tips_global_scope]
+  // Global (instance-wide) scope
+  // This computation runs at instance cold-start
+  public static final int INSTANCE_VAR = heavyComputation();


Moved to the very top. Not sure if we need the context listener.

functions/snippets/src/main/java/Tips.java

kurtisvg · 2019-08-24T21:03:11Z

functions/snippets/src/main/java/Tips.java

+  // [END functions_tips_scopes]
+  // [END run_tips_global_scope]
+
+  private static int lightComputation() {


These functions are still excluded, which means that this snippet isn't copy-paste-runnable (see the Sample Format Guidelines). Older samples might not be following the format, but we are trying to keep all new samples following the guidelines.

Also, both functions are exactly the same, which makes it a bit confusing to understand when reading. Perhaps it would be better to replace this with something like this:

// This function represents a heavy computation, which is only called when the instance starts. private static int heavyComputation() { TimeUnit.SECONDS.sleep(3); return System.currentTimeMillis(); } // This function represents a light computation, which might be called on every incoming request. private static int lightComputation() { TimeUnit.SECONDS.sleep(0); return System.currentTimeMillis(); }

These functions are excluded like they are in the Node sample.

Also, both functions are exactly the same

These functions are not exactly the same. They are modeled after Node.

https://github.com/GoogleCloudPlatform/nodejs-docs-samples/blob/4b2629e759eba9cf7d89aed3302bcf2795e1eecf/functions/tips/index.js#L18

What would you like us to do?

These functions are excluded like they are in the Node sample.

I believe that the node repo has a similar policy regarding copy-paste-runnable (since it's borrowed from the overarching Code Snippets Rubric). It looks like these samples probably predate this policy, but I don't know how the node repo is enforcing those rules.

Currently, the java-docs-samples policy is that new samples should follow the guidelines, while minor updates to existing samples can be grandfathered in. Since these samples are new, they need to follow the guidelines and include these functions so that users can copy them, paste them, and run them without having to fill out these functions.

These functions are not exactly the same.

This is my bad, the version I was looking at shows them both using addition, but I can see at HEAD they are different.

At the very least, this version of the sample should contain the comments I posted above. (My two cents is still that the intent of the sample is significantly more clear when using sleep, because the user can see that the difference between running those functions is non-trivial. However if you strongly prefer to remain consistent with the Node version, I'm satisfied with the comments clarifying above)

kurtisvg · 2019-08-24T21:07:43Z

functions/snippets/src/main/java/Tips.java

+  // [START run_tips_global_scope]
+  // Global (instance-wide) scope
+  // This computation runs at instance cold-start
+  public static final int INSTANCE_VAR = heavyComputation();


It should be updated to either use a ContextListener, or include a warning that global values in Servlets are not threadsafe, and a ContextListener is preferred for mutable variables.

I'm not really sure if/how a ContextListener would work with functions, so this snippet might be a good example to showcase that pattern as well.

grant · 2019-09-26T00:06:29Z

Fixed with #1586 instead.

feat: Add GCF Java Tips

70e8f43

grant requested review from kurtisvg and a team August 16, 2019 01:04

grant self-assigned this Aug 16, 2019

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 16, 2019

kurtisvg suggested changes Aug 16, 2019

View reviewed changes

Fix: Simplify PR

cc08120

grant commented Aug 23, 2019

View reviewed changes

kurtisvg suggested changes Aug 24, 2019

View reviewed changes

grant and others added 2 commits September 25, 2019 20:21

fix: Fix linter issues with functions_tips_scopes

c9b2ce7

Merge branch 'master' into grant_functions_tips

7f997d5

grant closed this Sep 26, 2019

kurtisvg deleted the grant_functions_tips branch November 15, 2019 21:36

feat: Add GCF Java Tips #1550

feat: Add GCF Java Tips #1550

Uh oh!

Conversation

grant commented Aug 16, 2019

Uh oh!

grant commented Aug 16, 2019

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grant 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grant Sep 6, 2019 • 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!

grant commented Sep 26, 2019

Uh oh!

Uh oh!

grant Sep 6, 2019 •

edited

Loading