-
Notifications
You must be signed in to change notification settings - Fork 695
Differentiate between Publisher and Subscriber TransportChannelProvider #2520
Conversation
Ensures that the correct client library defaults are applied for `maxInboundMessageSize`. Fixes: #2511.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to document the potentially breaking change -- if someone defined a transport channel without annotations, it will no longer be used.
|
||
TransportChannelProvider subscriberTcp = ctx.getBean("subscriberTransportChannelProvider", TransportChannelProvider.class); | ||
assertThat(FieldUtils.readField(subscriberTcp, "maxInboundMessageSize", true)) | ||
.isEqualTo(20 << 20); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define a 20MB constant maybe? Bit shifting is a bit obfuscated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a copy-paste from the client library.
We never documented that customization. So, I think it's fair to break it. I will add it to release notes though. |
91e3614
to
861f199
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but perhaps a follow-up issue to differentiate Topic/Subscriber admin functionality channel.
@@ -352,12 +353,12 @@ public TopicAdminClient topicAdminClient( | |||
@Bean | |||
@ConditionalOnMissingBean | |||
public TopicAdminSettings topicAdminSettings( | |||
TransportChannelProvider transportChannelProvider) { | |||
@Qualifier("publisherTransportChannelProvider") TransportChannelProvider publisherTransportChannelProvider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if admin functionality should have its own. The most obvious usecase is needing different credentials for admin operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that too, but I saw that they use exactly the same default for admin client.
https://github.com/googleapis/java-pubsub/blob/8cf77ee3b31356f342942a9ffaa61db2d0686769/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/SubscriptionAdminSettings.java#L227
Kudos, SonarCloud Quality Gate passed! 0 Bugs 100.0% Coverage 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. |
Codecov Report
@@ Coverage Diff @@
## master #2520 +/- ##
============================================
- Coverage 81.43% 74.22% -7.22%
+ Complexity 2383 2157 -226
============================================
Files 267 267
Lines 7737 7740 +3
Branches 799 799
============================================
- Hits 6301 5745 -556
- Misses 1098 1626 +528
- Partials 338 369 +31
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
Ensures that the correct client library defaults are applied for
maxInboundMessageSize
.Fixes: #2511.