WIP: Add GCF samples #1506

ace-n · 2019-07-09T23:00:45Z

DO NOT MERGE until these have been tested on Cloud Functions proper

ace-n · 2019-07-09T23:02:44Z

Note: these have only been tested with Java 8, as the Java 11 runtime itself isn't ready.

kurtisvg

There are existing samples under functions/snippets - these should be moved into that package and updated to match.

functions/pom.xml

functions/src/main/java/com/example/functions/HelloPubSubSample.java

functions/src/main/webapp/WEB-INF/web.xml

functions/src/main/webapp/guestbook.jsp

kurtisvg · 2019-07-10T15:37:15Z

functions/src/main/java/com/example/functions/GcsEvent.java

+// [START functions_helloworld_gcs_event]
+package com.example.functions;
+
+public class GcsEvent {


This needs to use getters (and setters).

What is the point of this class?

This is a struct that incoming GCS events are marshalled into.

(It's meant to be fairly read-only, so IMHO getters/setters would be overkill here.)

Java doesn't have have the concept of structs. It's idiomatic to use a constructor, private attributes, and getters to make it read-only.

kurtisvg · 2019-07-10T15:37:56Z

functions/src/main/java/com/example/functions/HelloGcsSample.java

+public class HelloGcsSample {
+  private static Logger logger = Logger.getLogger(HelloGcsSample.class.getName());
+
+  public void helloGcs(GcsEvent event) throws Exception {


This doesn't match the servlet signature.

Yes - AFAIK, GCF itself will marshal incoming GCS notifications into this format.

My concern here is that Java has a very strict typing on classes, so I'm concerned that GCF isn't actually going to use a com.example.functions.GcsEvent class and this sample won't work.

We plan to have the Java GSON parser to unmarshall the GcsEvent Pojo defined along with the user function. This experience aligns with the experience we currently provide users for Go. We also plan to open source the Java functions framework at Beta, so at that point an open source event type could be considered. However, I don't think this would be something we would consider as a blocker for Beta and GA.

Is the class going to be derived from the parameter for the function then?

Presumably if so, it would be safe to move GcsEvent as a static class inside the class with the function. This would allow us to keep the goal of having the sample copy/pastable.

lesv · 2019-10-21T20:33:13Z

Status please?

ace-n · 2019-10-21T20:34:10Z

This is on hold for now, due to product-side issues.

ace-n · 2020-01-22T19:59:55Z

FYI: this PR is back on my radar.

I'll go through and adjust these samples to conform with the new (Java 11) GCF function signatures.

ace-n · 2020-01-25T00:50:45Z

@kurtisvg LMK if you want me to apply your comments RE structs

cc @eamonnmcmanus

functions/pom.xml

averikitsch · 2020-01-27T23:59:55Z

functions/README.md

+
+# Google Cloud Functions Java Samples
+
+[Cloud Functions][functions_docs] is a lightweight, event-based, asynchronous


Do the docs have the instructions on how to run/deploy these?

Not yet - I think @labtopia (TW for GCF) is working on adding them, though.

averikitsch · 2020-01-28T00:00:40Z

functions/snippets/.gcloudignore

@@ -0,0 +1 @@
+src/test/


Is this file auto-generate?

No - but I didn't want the test code getting deployed to GCF when I use gcloud functions deploy. (Otherwise, it does unless you specifically exclude that directory in the deployment command.)

Could you expand on why you've chosen to do include this?

My understanding is that the Functions deployment deploys a jar? Are they compiled locally or after the code is deployed?

If it's compiled locally, I'm not sure this has any effect, and it's done remotely I'm not sure if it's a pattern we should push. As a user, if I've written tests, I want those tests to run on compile.

With GCF, you can deploy either a (self-sufficient) jar or a source hierarchy with pom.xml at the root. We expect users to deploy from source; the jar business is there to support build systems other than Maven. If you include the test source then your tests will be run remotely as part of your deployment. Whether you want that probably depends whether running them locally is enough, and on how long they take to run.

I think we should probably talk about this more in the GCF Java docs. We could also have a comment here saying what this does and why you might or might not want it. # comments are supported in .gcloudignore.

Since this doesn't appear to be the obvious default choice, I feel like it make the most sense to remove this for now. It seems like this this makes more sense to include in the GCF docs so the additional context makes clear it isn't required.

@kurtisvg any objection to adding a comment instead? Without this, gcloud functions deploy in the repo itself will deploy the tests by default.

I think that's the behavior most people will want though right? There might be some folks that don't want to run their tests on deploy, but I don't think it should be the default action.

From my perspective as a user, I would prefer to run gcloud functions deploy and have it run the tests + deploy rather than have to run mvn verify then run gcloud functions deploy.

functions/pom.xml

functions/snippets/pom.xml

functions/snippets/src/main/java/BackgroundEvent.java

functions/snippets/src/main/java/FileSystem.java

averikitsch · 2020-01-28T00:32:26Z

functions/snippets/src/main/java/RetrieveLogs.java


  private LoggingClient client;

  // Retrieve the latest Cloud Function log entries
-  public void retrieveLogs(HttpServletRequest request, HttpServletResponse response)
+  @Override
+  public void service(HttpRequest request, HttpResponse response)
      throws IOException {
    // Get the LoggingClient for the function.
    client = getClient();


@kurtisvg is this idiomatic?

functions/snippets/src/main/java/StackdriverLogging.java

averikitsch · 2020-01-28T00:36:35Z

Tests are failing

functions/snippets/src/test/java/HelloBackgroundSampleTest.java

functions/snippets/pom.xml

kurtisvg

Please split into smaller PRs. There is too much here to easily review in a single pass.

functions/pom.xml

functions/snippets/pom.xml

functions/snippets/src/main/java/GcsEvent.java

functions/snippets/src/main/java/HelloBackgroundSample.java

functions/snippets/src/main/java/Lazy.java

functions/snippets/src/main/java/ParseContentType.java

functions/snippets/src/main/java/SendHttpRequest.java

functions/snippets/src/test/java/HelloBackgroundSampleTest.java

eamonnmcmanus

FYI I think it would be great if we could arrange that some of these snippets were also used as the snippets that appear when you create a function in the Cloud Console. I've done some of the work for that to be possible and would be keen to do the rest. But it means that we'd probably want very skeletal snippets, and trying to make the same snippets do duty as both illustrative examples and starting points might not be the best approach.

functions/snippets/src/main/java/GcsEvent.java

functions/snippets/src/main/java/HelloGcs.java

functions/snippets/src/main/java/HelloHttp.java

functions/snippets/src/main/java/HelloPubSub.java

functions/snippets/src/main/java/SendHttpRequest.java

eamonnmcmanus · 2020-02-01T00:34:38Z

functions/snippets/src/main/java/PubSubMessage.java

@@ -0,0 +1,26 @@
+/*


I don't think the FF should have Google-specific classes like this. However there's a good case to be made for providing another artifact that defines them, rather than having everyone reinvent them.

functions/snippets/src/test/java/HelloBackgroundTest.java

functions/snippets/src/test/java/HelloGcsTest.java

kurtisvg · 2020-02-05T21:40:02Z

I just noticed that the directory organization doesn't match the normal convention (com.example.cloud.functions). @kurtisvg is this something we care about?

I think we decided that the consensus was to use com.example.*, but I don't believe we've codified it into the sample rules yet. The directory path should match the pom though.

eamonnmcmanus · 2020-02-06T19:51:59Z

I just noticed that the directory organization doesn't match the normal convention (com.example.cloud.functions). @kurtisvg is this something we care about?

I think we decided that the consensus was to use com.example.*, but I don't believe we've codified it into the sample rules yet. The directory path should match the pom though.

I'm not sure if this is connected, but I believe at one point we only supported functions in the default package (no package declaration). That restriction no longer applies, and using the default package tends to lead to weird problems, so we should eliminate it everywhere.

…docs-samples into gcf-ace

eamonnmcmanus

I think it would make sense to submit this. It might not be perfect, but we can always fix details later.

kurtisvg

Other outstanding issues:

Java 11 tests are currently failing
@averikitsch 's comment about using the correct package isn't resolved - you are still using the default package
Headers still need to be updated to 2020 for new files (tried to suggest all the ones I noticed but I may have missed some - please verify)

functions/snippets/pom.xml

kurtisvg · 2020-02-19T17:00:34Z

functions/snippets/.gcloudignore

@@ -0,0 +1 @@
+src/test/


Could you expand on why you've chosen to do include this?

My understanding is that the Functions deployment deploys a jar? Are they compiled locally or after the code is deployed?

If it's compiled locally, I'm not sure this has any effect, and it's done remotely I'm not sure if it's a pattern we should push. As a user, if I've written tests, I want those tests to run on compile.

functions/snippets/src/main/java/GcsEvent.java

functions/snippets/src/main/java/HelloGcs.java

functions/snippets/src/test/java/HelloPubSubIT.java

functions/snippets/src/test/java/SnippetsTests.java

…docs-samples into gcf-ace

kurtisvg

I think my remaining primary concerns:

It looks like the classes are still in the default package (undersrc/main/java). We should really move them into a proper package to avoid any unintended weirdness.
How to handle serialized events. I'm still not convinced there is a good reason to forgo best practices and skip getters and setters.

Other then those two things, I had one or two small nits.

kurtisvg · 2020-02-21T16:26:04Z

functions/snippets/.gcloudignore

@@ -0,0 +1 @@
+src/test/


Since this doesn't appear to be the obvious default choice, I feel like it make the most sense to remove this for now. It seems like this this makes more sense to include in the GCF docs so the additional context makes clear it isn't required.

functions/snippets/src/main/java/GcsEvent.java

kurtisvg · 2020-02-21T16:39:59Z

functions/snippets/src/test/java/HelloGcsTest.java

+
+  private EnvironmentVariables environmentVariables;
+
+  private static final TestLogHandler logHandler = new TestLogHandler();


Constant variables should be in all caps:

Suggested change

private static final TestLogHandler logHandler = new TestLogHandler();

private static final TestLogHandler LOG_HANDLER = new TestLogHandler();

Done throughout - marking these comments as resolved.

Actually (while we are nitting), Google Style does not say that all static final fields should be spelled with CONSTANT_CASE. Only truly immutable values should be, which TestLogHandler and Logger are not.

Ah interesting. I didn't know the Google Style Guide specified that - I've always just followed the traditional Java conventions where anything final is considered a constant since the reference is immutable.

functions/snippets/src/test/java/HelloGcsTest.java

functions/snippets/src/test/java/HelloBackgroundTest.java

functions/snippets/src/test/java/MockHttpResponse.java

kurtisvg

LGTM once we reach a decision on the .gcloudignore

kurtisvg

LGTM once we reach a decision on the .gcloudignore

ace-n · 2020-02-25T23:41:59Z

FYI: I'm merging this for now, since all local tests are passing.

b/150242638 is blocking me from testing it on GCF, so I'm prepared to make changes later if issues come up.

ace-n added 5 commits May 29, 2019 14:01

helloHttp + helloPubSub samples + test scaffolding

30766ac

Rename to functions + add missing scaffolding code

4467d41

Add GCS samples

c267210

Add region tags

5083e04

Update servlet version

9813eb5

ace-n added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Jul 9, 2019

ace-n requested review from averikitsch, kurtisvg and a team July 9, 2019 23:00

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

kurtisvg self-assigned this Jul 9, 2019

kurtisvg suggested changes Jul 10, 2019

View reviewed changes

Address kvg's comments

e707a07

ace-n mentioned this pull request Aug 15, 2019

Feat: Adds functions_helloworld_background #1549

Merged

Ace Nassri added 3 commits January 22, 2020 12:00

Merge branch 'master' into gcf-ace

f50e93f

Compress directory structure

d409a3e

Sample updates

8d0507b

Ace Nassri added 2 commits January 24, 2020 16:50

Merge branch 'master' into gcf-ace

281b464

Minor tweaks to make everything work

2a6f3d1

averikitsch reviewed Jan 28, 2020

View reviewed changes

eamonnmcmanus requested changes Jan 28, 2020

View reviewed changes

functions/snippets/src/test/java/HelloBackgroundSampleTest.java Outdated Show resolved Hide resolved

functions/snippets/pom.xml Outdated Show resolved Hide resolved

kurtisvg suggested changes Jan 28, 2020

View reviewed changes

Address comments, but could not run checkstyle locally

88be136

eamonnmcmanus requested changes Feb 1, 2020

View reviewed changes

ace-n added 2 commits February 14, 2020 16:44

Address comments, take 3

52a4bf0

Merge branch 'gcf-ace' of http://github.com/GoogleCloudPlatform/java-…

3be0f12

…docs-samples into gcf-ace

eamonnmcmanus reviewed Feb 18, 2020

View reviewed changes

Merge branch 'master' into gcf-ace

7263968

eamonnmcmanus approved these changes Feb 18, 2020

View reviewed changes

ace-n requested a review from kurtisvg February 18, 2020 23:23

kurtisvg suggested changes Feb 19, 2020

View reviewed changes

ace-n added 3 commits February 20, 2020 11:51

Address comments, take 4

204993a

Merge branch 'gcf-ace' of http://github.com/GoogleCloudPlatform/java-…

3138631

…docs-samples into gcf-ace

Take 4a: pom.xml tweak

84c5dba

ace-n requested review from kurtisvg and averikitsch February 20, 2020 21:19

ace-n added 2 commits February 20, 2020 14:40

Take 4b: fix mungled pom.xml

e4defc9

Merge branch 'master' into gcf-ace

455eef6

kurtisvg suggested changes Feb 21, 2020

View reviewed changes

Address comments, take 5

fdfa27b

ace-n requested a review from kurtisvg February 21, 2020 21:28

Merge branch 'master' into gcf-ace

9a68ddc

kurtisvg suggested changes Feb 21, 2020

View reviewed changes

kurtisvg approved these changes Feb 21, 2020

View reviewed changes

Add notes to gcloudignore

5856ca1

averikitsch approved these changes Feb 21, 2020

View reviewed changes

Ace Nassri added 3 commits February 21, 2020 15:14

Merge branch 'master' into gcf-ace

4d8f1e4

Merge branch 'master' into gcf-ace

57d85fc

Merge branch 'master' into gcf-ace

dbc2ce6

ace-n merged commit 30f56ef into master Feb 25, 2020

ace-n deleted the gcf-ace branch February 25, 2020 23:42


		# Google Cloud Functions Java Samples

		[Cloud Functions][functions_docs] is a lightweight, event-based, asynchronous


		private EnvironmentVariables environmentVariables;

		private static final TestLogHandler logHandler = new TestLogHandler();

	private static final TestLogHandler logHandler = new TestLogHandler();
	private static final TestLogHandler LOG_HANDLER = new TestLogHandler();

		@@ -0,0 +1 @@
		src/test/

		@@ -0,0 +1 @@
		src/test/

		@@ -0,0 +1 @@
		src/test/

WIP: Add GCF samples #1506

WIP: Add GCF samples #1506

Uh oh!

Conversation

ace-n commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ace-n commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kurtisvg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ace-n Jul 18, 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!

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!

lesv commented Oct 21, 2019

Uh oh!

ace-n commented Oct 21, 2019

Uh oh!

ace-n commented Jan 22, 2020

Uh oh!

ace-n commented Jan 25, 2020

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

averikitsch commented Jan 28, 2020

Uh oh!

Uh oh!

Uh oh!

kurtisvg left a comment

Choose a reason for hiding this comment

ace-n commented Jul 9, 2019 •

edited

Loading

ace-n commented Jul 9, 2019 •

edited

Loading

ace-n Jul 18, 2019 •

edited

Loading