Pubsub stream binder via synchronous pull #1419

elefeint · 2019-01-30T15:39:22Z

Add PubSubMessageSource integration to PubSubMessageChannelBinder.

Fixes #1365.

artembilan

So, far so good.

I don't see any obstacles to merge this just after #1321

Thanks

...es/spring-cloud-gcp-pubsub-polling-binder-sample/src/main/java/com/example/PollableSink.java

...les/spring-cloud-gcp-pubsub-polling-binder-sample/src/main/java/com/example/SinkExample.java

...main/java/org/springframework/cloud/gcp/stream/binder/pubsub/PubSubMessageChannelBinder.java

Other changes: * Modified sample Pub/Sub Sender app to accept the number of times to send the message and append sequential message number to each copy. * Split sample Pub/Sub Receiver app into StreamingPull (original behavior) and SyncPull (new implementation) configuration subclasses.

Co-Authored-By: elefeint <41136058+elefeint@users.noreply.github.com>

elefeint · 2019-02-01T16:49:18Z

I am going to fix that commit history. I've created the branch off of pubsub-pull, which caused commits to be duplicated after rebase.

codecov-io · 2019-02-01T17:27:47Z

Codecov Report

Merging #1419 into master will increase coverage by 0.46%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master    #1419      +/-   ##
============================================
+ Coverage     68.57%   69.03%   +0.46%     
- Complexity     1475     1488      +13     
============================================
  Files           209      205       -4     
  Lines          5826     5846      +20     
  Branches        585      593       +8     
============================================
+ Hits           3995     4036      +41     
+ Misses         1595     1571      -24     
- Partials        236      239       +3

Flag	Coverage Δ	Complexity Δ
#integration	`?`	`?`
#unittests	`69.03% <0%> (+0.46%)`	`1488 <0> (+13)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
...ream/binder/pubsub/PubSubMessageChannelBinder.java	`72.22% <0%> (-9.03%)`	`5 <0> (ø)`
...pringframework/cloud/gcp/core/util/MapBuilder.java	`88.88% <0%> (-11.12%)`	`3% <0%> (ø)`
...oud/gcp/data/datastore/core/DatastoreTemplate.java	`91.98% <0%> (-0.15%)`	`102% <0%> (+13%)`
...ta/datastore/core/DatastoreTransactionManager.java	`74.24% <0%> (ø)`	`15% <0%> (ø)`	⬇️
...-sample/src/main/java/com/example/UserMessage.java
...main/java/com/example/PubSubBinderApplication.java
...-sample/src/main/java/com/example/SinkExample.java
...ample/src/main/java/com/example/SourceExample.java

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 1c75505...3604135. Read the comment docs.

artembilan

Only one concern, plus the Copyright in the PubSubMessageChannelBinder has to be updated.

artembilan · 2019-02-02T21:33:16Z

...oud-gcp-pubsub-polling-binder-sample/src/test/java/com/example/SampleAppIntegrationTest.java

+
+		this.restTemplate.postForObject("/newMessage", map, String.class);
+
+		Callable<Boolean> logCheck = () -> baos.toString().contains("New message received from testUserName via polling: " + message + " at ");


Is this message not finished or am I missing something?

" at " would be followed by a timestamp, which is not verifiable without mocking the clock. I am removing " at " from the verification string, since it looks confusing without adding value.

meltsufin

Do we need some refdoc updates, at least to say we support this for the binder and link to the sample?

meltsufin · 2019-02-04T17:05:34Z

spring-cloud-gcp-samples/spring-cloud-gcp-pubsub-polling-binder-sample/README.adoc

+The sample app prompts a user for a message and their user name.
+That data is added to a `UserMessage` object, together with the time of message creation, and is sent through Google Cloud Pub/Sub to a sink which simply logs the message.
+
+If the topic and subscription for the sink and source don't exist, the binder will automatically create them in Google Cloud Pub/Sub based on the values in link:src/main/resources/application.properties[application.properties].


Maybe point out the key pieces of code here in the description?

meltsufin

Great!

artembilan suggested changes Jan 30, 2019

View reviewed changes

elefeint and others added 27 commits February 1, 2019 09:10

Add docs

b7dc9e9

style feedback

8ef9b84

Use @ConditionalOnProperty instead of manual configuration

645372a

Use state assertion

6c9704b

cache a batch of messages

6ab0d13

add test

8a1e4ba

split sample receiver apps; licence year

e216e72

auto acking/nacking

9002dfc

add user setting to block on pull request

64cba40

typo

2b759e7

Co-Authored-By: elefeint <41136058+elefeint@users.noreply.github.com>

fix docs

1cfd827

remove manual thread sleep

99d3d90

Update docs/src/main/asciidoc/spring-integration-pubsub.adoc

9f90ab8

Co-Authored-By: elefeint <41136058+elefeint@users.noreply.github.com>

remove maxfetchsize from constructor

4e8c2f4

set default pull size to 1

5ad82c2

make acknowledgement callback handle manual mode, too

065ed03

fix version and tests

2da9801

bump versions

3b465d5

remove delay parameter from samples

1719a6e

rename setter to setBlockOnPull

bc94b64

address feedback

9fb7303

wip polling binder implementation and sample

a9ff345

add content handling, acking

8625d05

cleanup

9547581

update licence year

ec0b02a

reduce sample boilerplate

2ad3c3d

elefeint force-pushed the pubsub-pull-stream-binder branch from 46ebad3 to 2ad3c3d Compare February 1, 2019 16:42

elefeint changed the base branch from pubsub-pull to master February 1, 2019 16:43

elefeint added 6 commits February 1, 2019 12:07

wip polling binder implementation and sample

2ddb875

add content handling, acking

b0a4646

cleanup

0d48758

reduce sample boilerplate

c38fad7

bring in remaining differences from master

1a7dfbc

remove real topic/subscription name

7457c6e

fix sonar issues

de1be6a

elefeint changed the title ~~[WIP] Pubsub stream binder via synchronous pull~~ Pubsub stream binder via synchronous pull Feb 1, 2019

elefeint requested a review from meltsufin February 1, 2019 18:45

artembilan suggested changes Feb 2, 2019

View reviewed changes

fix copyright and unconfuse test string

4205761

artembilan previously approved these changes Feb 4, 2019

View reviewed changes

meltsufin reviewed Feb 4, 2019

View reviewed changes

elefeint mentioned this pull request Feb 4, 2019

Change PubSubMessageSource default ack mode to AUTO #1431

Merged

add documentation

3604135

elefeint dismissed artembilan’s stale review via 3604135 February 5, 2019 15:29

artembilan approved these changes Feb 5, 2019

View reviewed changes

meltsufin approved these changes Feb 5, 2019

View reviewed changes

elefeint merged commit 6d8eda9 into master Feb 5, 2019

elefeint deleted the pubsub-pull-stream-binder branch February 5, 2019 16:03

elefeint mentioned this pull request Feb 5, 2019

Create a polled stream binder consumer using synchronous pull #1365

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pubsub stream binder via synchronous pull #1419

Pubsub stream binder via synchronous pull #1419

elefeint commented Jan 30, 2019 •

edited

artembilan left a comment

elefeint commented Feb 1, 2019

codecov-io commented Feb 1, 2019 •

edited

artembilan left a comment

artembilan Feb 2, 2019

elefeint Feb 4, 2019

meltsufin left a comment

meltsufin Feb 4, 2019

meltsufin left a comment


		this.restTemplate.postForObject("/newMessage", map, String.class);

		Callable<Boolean> logCheck = () -> baos.toString().contains("New message received from testUserName via polling: " + message + " at ");

Pubsub stream binder via synchronous pull #1419

Pubsub stream binder via synchronous pull #1419

Conversation

elefeint commented Jan 30, 2019 • edited

artembilan left a comment

Choose a reason for hiding this comment

elefeint commented Feb 1, 2019

codecov-io commented Feb 1, 2019 • edited

Codecov Report

artembilan left a comment

Choose a reason for hiding this comment

artembilan Feb 2, 2019

Choose a reason for hiding this comment

elefeint Feb 4, 2019

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

meltsufin Feb 4, 2019

Choose a reason for hiding this comment

meltsufin left a comment

Choose a reason for hiding this comment

elefeint commented Jan 30, 2019 •

edited

codecov-io commented Feb 1, 2019 •

edited