Pubsub flexible sample by jabubake · Pull Request #592 · GoogleCloudPlatform/java-docs-samples

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

+ 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

+ // 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

+
+# [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

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


What does the '3' represent?

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

+## 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

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


Is this line necessary?

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

+ 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

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


New sample change year to 2016.

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

+ // 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

+ * @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

+
+## 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

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


Add a new line.

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

@@ -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

+ }
+ }
+
+ // ...


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

+ </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

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


Cleaner solution 👍

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

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


Clean up whitespace

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

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


Add a new line.

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

+
+## 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

+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 Natividadand 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 masterApr 25, 2017

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

olivi-eh 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" %>

+ }
+ }
+ // ...

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!

Reviewers

Assignees

Labels

codecov-io commented Apr 20, 2017•
edited
Loading