-
Notifications
You must be signed in to change notification settings - Fork 313
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: set maxInboundMetadataSize
in pubsub
#3157
Conversation
maxInboundMetadataSize
in pubsub
Native tests against storage sample failed:
This is irrelevant to this PR. |
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.
Do you mind adding a comment citing the pubsub client library default?
Also, do you think this default behavior is worth mentioning somewhere in the documentation?
Do you mean in SCGCP docs? |
Yes, I am referring to SCGCP docs. In light of "secure by defaults", although we are hardcoding in this case, IMO it probably still worth explicitly spell out that we are following PubSub client library defaults for these settings. Another question, should we also set "maxInboundMessageSize" as client library does here? And what about |
@@ -436,6 +436,9 @@ public SubscriptionAdminClient subscriptionAdminClient( | |||
@ConditionalOnMissingBean(name = "subscriberTransportChannelProvider") | |||
public TransportChannelProvider subscriberTransportChannelProvider() { | |||
return SubscriberStubSettings.defaultGrpcTransportProviderBuilder() | |||
// default value specified by pubsub client library, | |||
// see https://github.com/googleapis/java-pubsub/blob/main/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/Subscriber.java#L487. | |||
.setMaxInboundMetadataSize(4 * 1024 * 1024) |
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.
Is it possible to reference the default variable here instead of hardcoding the default value? Just so that we are aligned with the default in the client in case it changes in the future.
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.
The default value is a private variable that can't be referenced (source).
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.
Thanks for checking, That is a little unfortunate that it can't be referenced at the moment. It might be worth creating a ticket in the client repo (similar to googleapis/java-pubsub#373)
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 think the builder is autogenerated by the generator: https://github.com/googleapis/java-pubsub/blob/main/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/SubscriptionAdminSettings.java#L246 and https://github.com/googleapis/java-pubsub/blob/main/google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/v1/stub/SubscriberStubSettings.java#L425
I think |
+1 how about we document this in https://googlecloudplatform.github.io/spring-cloud-gcp/reference/html/index.html#publishersubscriber-configuration? |
I didn't add this property in GcpPubSubProperties and I don't see I think we shouldn't surface this property in the doc, what do you think? |
Agreed that this particular property can't be configured so adding it as part of the configurations table might be confusing but I think users can still customize the
|
Added a note in the pubsub doc. |
Quality Gate passedIssues Measures |
Fix: #2785