Pubsub flexible sample #592

jabubake · 2017-04-06T04:58:22Z

Flex pubsub push endpoint sample with Datastore persistence.
Tests : TODO (given we're consolidating the flex apps for testing, removed the existing ones for now for this sample)

* Updated timeout for PubSubPush

- changed com/example/appengine/pubsub to com/example/flexible/pubsub - added Apache License - removed unnecessary dependencies

lesv · 2017-04-06T17:10:53Z

@jabubake Circle failed :(

frankyn

Overall, it's looking great. I commented on a few things.

Style check failed in CircleCI
The sample is missing tests.

frankyn · 2017-04-06T06:37:02Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubPush.java

+    try {
+      messageRepository.save(message);
+      // 200, 201, 204, 102 status codes are interpreted as success by the Pub/Sub system
+      resp.setStatus(200);


200 -> HttpServletResponse.SC_OK

frankyn · 2017-04-06T06:37:44Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubPush.java

+      // 200, 201, 204, 102 status codes are interpreted as success by the Pub/Sub system
+      resp.setStatus(200);
+    } catch (Exception e) {
+      resp.setStatus(500);


500 -> HttpServletResponse.SC_INTERNAL_SERVER_ERROR

frankyn · 2017-04-12T23:52:07Z

flexible/pubsub/src/main/appengine/app.yaml

+
+# [START env_variables]
+env_variables:
+  PUBSUB_TOPIC: test-topic-1


The values should reflect the README for this sample.
export PUBSUB_TOPIC=
export PUBSUB_VERIFICATION_TOKEN=
export PUBSUB_SUBSCRIPTION_ID=

frankyn · 2017-04-13T00:01:27Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubService.java

+    try (SubscriptionAdminClient subscriberAdminClient = SubscriptionAdminClient.create()) {
+      try {
+        subscriberAdminClient.createSubscription(
+            subscriptionName, getTopicName(topicId), pushConfig, 3);


What does the '3' represent?

frankyn · 2017-04-13T00:46:19Z

flexible/pubsub/README.md

+## Run
+
+Set the following environment variables and run using shown Maven command. You can then
+direct your browser to  `http://localhost:8080/`


Double space next to "browser to[SPACE][SPACE]"

frankyn · 2017-04-13T01:15:02Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubService.java

+    private static final PubSubService INSTANCE;
+
+    static {
+      System.out.println("instance");


Is this line necessary?

frankyn · 2017-04-13T01:16:48Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubService.java

+  private static class LazyInit {
+    private static final PubSubService INSTANCE;
+
+    static {


After reading into this Lazy initialization singleton pattern. I wonder if there's a more readable solution? This wasn't clear during my first pass.

frankyn · 2017-04-13T01:17:33Z

flexible/pubsub/pom.xml

@@ -0,0 +1,114 @@
+<!--
+  Copyright 2016 Google Inc. All Rights Reserved.


New sample change year to 2016.

frankyn · 2017-04-13T01:27:23Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/MessageRepository.java

+    // Get Message saved in Datastore
+    Datastore datastore = getDatastoreInstance();
+    Query<Entity> query =
+        Query.newEntityQueryBuilder()


This should fit in the previous line.

frankyn · 2017-04-13T01:34:11Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubService.java

+   * @param endpoint push endpoint URL
+   * @throws Exception
+   */
+  private void addPushEndPoint(String topicId, String subscriptionId, String endpoint)


This method wasn't easy to read. Can you make this method be more descriptive?

codecov-io · 2017-04-20T20:41:37Z

Codecov Report

Merging #592 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #592   +/-   ##
=========================================
  Coverage     55.84%   55.84%           
  Complexity      202      202           
=========================================
  Files            74       74           
  Lines          2052     2052           
  Branches        131      131           
=========================================
  Hits           1146     1146           
  Misses          874      874           
  Partials         32       32

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 5b63acb...7256754. Read the comment docs.

frankyn

Looking good! I have a few minor nits.

frankyn · 2017-04-25T14:40:03Z

flexible/pubsub/README.md

+
+## Deploy
+
+Update the environment variables `PUBSUB_TOPIC` and `PUBSUB_VERIFICATION_TOKEN` in `app.yaml`,


provide a link to the app.yaml or a full path. src/main/appengine/app.yaml

frankyn · 2017-04-25T14:49:40Z

flexible/pubsub/src/main/webapp/index.jsp

+      <%= PubSubHome.getReceivedMessages() %>
+    </table>
+  </body>
+</html>


Add a new line.

frankyn · 2017-04-25T14:50:25Z

flexible/pubsub/src/main/webapp/index.jsp

@@ -0,0 +1,21 @@
+<%@ page import="com.example.flexible.pubsub.PubSubHome" %>


One thing I noticed is, I had to refresh the page to see an updated list of messages. What do you think about adding an auto-refresh every 10 seconds or so?

frankyn · 2017-04-25T14:51:59Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubPublish.java

+    }
+  }
+
+  // ...


Why did you add this line?

I was trying to show that it was an incomplete class but it seems useless, removed.

frankyn · 2017-04-25T14:55:01Z

flexible/pubsub/pom.xml

+  </parent>
+
+  <properties>
+    <appengine.maven.plugin>1.0.0</appengine.maven.plugin>


I'm sure this will be auto-updated once merged, but this version has updated to 1.3.0.

Good catch, done.

frankyn · 2017-04-25T15:06:34Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/PubSubHome.java

+   *
+   * @return html representation of messages (one per row)
+   */
+  public static String getReceivedMessages() {


Cleaner solution 👍

frankyn · 2017-04-25T15:07:57Z

flexible/pubsub/src/main/java/com/example/flexible/pubsub/MessageRepository.java

+   */
+  List<Message> retrieve(int limit);
+}
+


Clean up whitespace

frankyn · 2017-04-25T15:08:24Z

flexible/pubsub/pom.xml

+    </plugins>
+  </build>
+</project>
+<!-- [END project] -->


Add a new line.

frankyn · 2017-04-25T15:09:04Z

flexible/pubsub/README.md

+
+## Setup
+
+Make sure `gcloud` is installed and authenticated.


Add a link to Cloud SDK https://cloud.google.com/sdk/docs/

frankyn · 2017-04-25T15:15:42Z

flexible/pubsub/README.md

+gcloud beta pubsub topics create <your-topic-name>
+```
+
+Create a subscription, which includes specifying the endpoint to which the Pub/Sub server should send requests.


Maybe change this to:

Create a push subscription, to send pushed messages to a Google Cloud Project URL such as https://<your-project-id>.appspot.com/.

Done. That does sound better.

frankyn

LGTM, thanks!

…3.0 (#592) [![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/GoogleCloudPlatform/cloud-opensource-java)) | `25.2.0` -> `25.3.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.3.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.3.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.3.0/compatibility-slim/25.2.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/25.3.0/confidence-slim/25.2.0)](https://docs.renovatebot.com/merge-confidence/) | --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-notification).

Frank Natividad and others added 18 commits April 5, 2017 21:48

Adding base code for PubSub example

0722c03

Changed path from managedvms to appengine

3baa2d9

Updating code style and added exponential backoff timeout

33fd597

Added Test for PubSubHome

b674933

Removed underscore before IT

79330cb

Suppressed abbreviation warning and removed whitespace

b4f932a

Updating pom.xml with necessary plugins for tests

0a7781b

* Updated timeout for PubSubPush

Removed underscore from test method name

c60b4d9

Added tests for PubSubPlubish and PubSubPush

0bd8196

Refactored string literals to a named variable

3962632

Added README.md with sample message to test PubSub push subscription

2f5771d

Refactored variable instantiations and cleaned up pom.xml

411289d

Refactored trySaveMessage and createMessageList into a single method

873dbf5

Updated pom project information

88048cf

Code clean up and directory change

a964fc3

- changed com/example/appengine/pubsub to com/example/flexible/pubsub - added Apache License - removed unnecessary dependencies

Changed year from 2015 to 2016.

54c3798

pubsub flex sample rewrite

c5c972b

updating readme

dc771c0

jabubake requested a review from frankyn April 6, 2017 04:58

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 6, 2017

frankyn suggested changes Apr 13, 2017

View reviewed changes

jabubake added 4 commits April 18, 2017 14:38

simplifying code

fb45c47

cleanup, adding doc tags

ba77db1

Merge branch 'master' into pubsub-flexible-sample

dee7131

bug fixes

2fe47ac

jabubake added 2 commits April 20, 2017 16:42

bug fixes, adding PubSubHome

58bf601

removing secure always

69c52b8

frankyn suggested changes Apr 25, 2017

View reviewed changes

review based fixes

f9258ef

frankyn approved these changes Apr 25, 2017

View reviewed changes

jabubake added 2 commits April 25, 2017 11:42

Merge branch 'master' into pubsub-flexible-sample

329ae0f

Merge branch 'master' into pubsub-flexible-sample

7256754

jabubake merged commit 4514e8b into master Apr 25, 2017

jabubake deleted the pubsub-flexible-sample branch April 25, 2017 22:55

bourgeoisor mentioned this pull request Nov 11, 2022

migrate code from googleapis/java-notification #7406

Closed

9 tasks

		@@ -0,0 +1,114 @@
		<!--
		Copyright 2016 Google Inc. All Rights Reserved.


		## Deploy

		Update the environment variables `PUBSUB_TOPIC` and `PUBSUB_VERIFICATION_TOKEN` in `app.yaml`,

		@@ -0,0 +1,21 @@
		<%@ page import="com.example.flexible.pubsub.PubSubHome" %>

+                  }
+                }
+                // ...

Pubsub flexible sample #592

Pubsub flexible sample #592

Uh oh!

Conversation

jabubake commented Apr 6, 2017

Uh oh!

lesv commented Apr 6, 2017

Uh oh!

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

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!

codecov-io commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

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

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!

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!

frankyn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov-io commented Apr 20, 2017 •

edited

Loading