Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Control sync/async publish in Spring Cloud Stream binder #2473

Merged
merged 3 commits into from
Jul 24, 2020

Conversation

elefeint
Copy link
Contributor

Expose the Spring Integration PubSubMessageHandler's sync property via extended binder properties. Turning async mode off is useful when errors must be exposed to sender.

There is currently no way to force synchronous sending through Cloud Pub/Sub Spring Cloud Stream binder.

@elefeint elefeint requested review from meltsufin and dzou July 22, 2020 18:53
@codecov
Copy link

codecov bot commented Jul 22, 2020

Codecov Report

Merging #2473 into master will decrease coverage by 7.20%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2473      +/-   ##
============================================
- Coverage     81.42%   74.22%   -7.21%     
+ Complexity     2377     2154     -223     
============================================
  Files           267      267              
  Lines          7722     7727       +5     
  Branches        798      798              
============================================
- Hits           6288     5735     -553     
- Misses         1095     1622     +527     
- Partials        339      370      +31     
Flag Coverage Δ Complexity Δ
#integration ? ?
#unittests 74.22% <100.00%> (+0.02%) 2154.00 <2.00> (+4.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...ream/binder/pubsub/PubSubMessageChannelBinder.java 80.00% <100.00%> (+0.83%) 6.00 <0.00> (ø)
...er/pubsub/properties/PubSubProducerProperties.java 100.00% <100.00%> (ø) 3.00 <2.00> (+2.00)
...gcp/secretmanager/SecretManagerPropertySource.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...a/spanner/repository/query/SpannerQueryMethod.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-6.00%)
...retmanager/SecretManagerPropertySourceLocator.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...figure/config/GcpConfigBootstrapConfiguration.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...e/spanner/GcpSpannerEmulatorAutoConfiguration.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...epository/config/SpannerRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...ository/config/DatastoreRepositoriesRegistrar.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...restore/GcpFirestoreEmulatorAutoConfiguration.java 0.00% <0.00%> (-84.85%) 0.00% <0.00%> (-4.00%)
... and 57 more

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 48a5813...1c4c850. Read the comment docs.

@@ -24,4 +24,13 @@
* @author Chengyuan Zhao
*/
public class PubSubProducerProperties extends PubSubCommonProperties {
private boolean sync;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While uninitialized defaults to false for boolean, it might be nice to just have sync = false to be explicit on the defaults

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do.

@@ -71,4 +94,25 @@ public void testAfterUnbindConsumer() {
verify(this.channelProvisioner).afterUnbindConsumer(this.consumerDestination);
}

@Test
public void producerSyncPropertyFalseByDefault() {
Copy link
Contributor

@dzou dzou Jul 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's possible, I think it would be better to set the property via spring.cloud.stream.gcp.pubsub.default.producer.sync rather than set the setting through the property class directly. Mainly to verify that spring.cloud.stream.gcp.pubsub.default.producer.sync is indeed the correct format of the property name (as I am not sure what the correct form is myself).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I've been getting increasingly uncomfortable with the amount of mocking in this test. I'll either move this into an integration test, or turn this test into a light integration test with only a mocked pubsub template.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the best I could do without testing SCS implementation details. There is still one mocked transition from our properties into a SCS wrapper, but at least it tests the property mapping correctly.

@elefeint elefeint requested a review from dzou July 24, 2020 01:00
@sonarcloud
Copy link

sonarcloud bot commented Jul 24, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

warning The version of Java (1.8.0_151) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

Copy link
Contributor

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@elefeint elefeint merged commit 8df7b52 into master Jul 24, 2020
@elefeint elefeint deleted the binder-add-async-override branch July 24, 2020 13:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants