Skip to content
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

Merged
merged 6 commits into from
Oct 3, 2024

Conversation

JoeWang1127
Copy link
Contributor

Fix: #2785

@JoeWang1127 JoeWang1127 changed the title fix: set maxInboundMetadataSize fix: set maxInboundMetadataSize in pubsub Aug 23, 2024
@JoeWang1127
Copy link
Contributor Author

Native tests against storage sample failed:

Error: Classes that should be initialized at run time got initialized during image building:
 org.apache.commons.logging.LogFactory was unintentionally initialized at build time. To see why org.apache.commons.logging.LogFactory got initialized use --trace-class-initialization=org.apache.commons.logging.LogFactory
org.apache.commons.logging.LogFactoryService was unintentionally initialized at build time. To see why org.apache.commons.logging.LogFactoryService got initialized use --trace-class-initialization=org.apache.commons.logging.LogFactoryService
To see how the classes got initialized, use --trace-class-initialization=org.apache.commons.logging.LogFactory,org.apache.commons.logging.LogFactoryService

This is irrelevant to this PR.

@JoeWang1127 JoeWang1127 marked this pull request as ready for review August 26, 2024 13:34
@JoeWang1127 JoeWang1127 requested a review from a team as a code owner August 26, 2024 13:34
Copy link
Contributor

@zhumin8 zhumin8 left a 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?

@JoeWang1127
Copy link
Contributor Author

Also, do you think this default behavior is worth mentioning somewhere in the documentation?

Do you mean in SCGCP docs?

@zhumin8
Copy link
Contributor

zhumin8 commented Aug 26, 2024

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 publisherTransportChannelProvider setting? At a glance, it looks like SCGCP is not keeping consistent with client library defaults here?

@@ -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)
Copy link
Contributor

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.

Copy link
Contributor Author

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).

Copy link
Contributor

@mpeddada1 mpeddada1 Aug 26, 2024

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JoeWang1127
Copy link
Contributor Author

Another question, should we also set "maxInboundMessageSize" as client library does here?

maxInboundMessageSize is set here.

And what about publisherTransportChannelProvider setting? At a glance, it looks like SCGCP is not keeping consistent with client library defaults here?

I think publisherTransportChannelProvider should be another issue.

@mpeddada1
Copy link
Contributor

Also, do you think this default behavior is worth mentioning somewhere in the documentation?

+1 how about we document this in https://googlecloudplatform.github.io/spring-cloud-gcp/reference/html/index.html#publishersubscriber-configuration?

@JoeWang1127
Copy link
Contributor Author

+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 TransportChannelProvider can be custom configured in the existing pubsub properties.

I think we shouldn't surface this property in the doc, what do you think?

@mpeddada1
Copy link
Contributor

+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 TransportChannelProvider can be custom configured in the existing pubsub properties.

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 subscriberTransportChannelProvider by providing their own bean. It might still be worth adding "Note" section in the documentation which mentions this default value? As an example, we have a note that mentions the default thread names we use:

By default, subscription-specific threads are named after fully-qualified subscription name, ex: gcp-pubsub-subscriber-projects/project-id/subscriptions/subscription-name. This can be customized, by registering a SelectiveSchedulerThreadNameProvider bean.

@JoeWang1127
Copy link
Contributor Author

It might still be worth adding "Note" section in the documentation which mentions this default value

Added a note in the pubsub doc.

Copy link

sonarcloud bot commented Oct 3, 2024

@JoeWang1127 JoeWang1127 merged commit f333e41 into main Oct 3, 2024
70 of 76 checks passed
@JoeWang1127 JoeWang1127 deleted the fix/restore-pubsub-default branch October 3, 2024 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TransportChannelProvider bean overwrites maxInboundMetadataSize with null
3 participants