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

Customize thread name prefixes for subscription specific TaskSchedulers #1152

Merged

Conversation

poznachowski
Copy link
Contributor

Currently thread names for subscription specific Task Schedulers are lengthy as they comprise of
gcp-pubsub-subscriber- + fully qualified subscription name, ex:
gcp-pubsub-subscriber-projects/fake project/subscriptions/subscription-name1

This may result in a lengthy log statement, if thread name is a part of it.

This MR allows customization of subscription specific Task Scheduler's thread names.

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

It's a nice improvement, but unfortunately accepting a bean in GcpPubSubAutoConfiguration constructor that is defined in the same autoconfiguration class leads to a circular reference. You can test this by starting up the sample app.

I see two ways around this:

  1. creating defaultSelectiveSchedulerThreadNameProvider in a new autoconfiguration class,
  2. not providing a default bean at all and instead accepting Optional<SelectiveSchedulerThreadNameProvider> selectiveSchedulerThreadNameProvider in the constructor. Then in registerSelectiveSchedulerBeans, the default would end up hardcoded as before:
        String threadName = selectiveSchedulerThreadNameProvider.isPresent() ? 
            selectiveSchedulerThreadNameProvider.get().getThreadName(fullSubscriptionName) : 
            "gcp-pubsub-subscriber-" + qualifiedName;

The second way seems simpler to me, but it's your call.

Also, could you turn on google style and reformat? mvn validate will run style checks to make sure it's good.

@poznachowski
Copy link
Contributor Author

You're obviously right, sorry for that.
I went the latter route, but used Spring's ObjectProvider as I find it more consistent with other autoconfiguration classes I've seen. Even GcpPubSubAutoconfiguration uses ObjectProvider a lot.
mvn validate runs successfully now.

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Looks great!
Would you mind adding a line or two to the Subscription-specific Configurations section in the reference documentation?

@poznachowski
Copy link
Contributor Author

Sure, reference added. Please check if that concludes the change.

Copy link
Contributor

@elefeint elefeint left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@sonarcloud
Copy link

sonarcloud bot commented Jun 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@elefeint elefeint merged commit 55c05f5 into GoogleCloudPlatform:main Jun 6, 2022
@poznachowski poznachowski deleted the customize_thread_names branch June 6, 2022 18:52
@poznachowski
Copy link
Contributor Author

Hey @elefeint ! I don't know where to ask this question elsewhere - Do you have any ETA for publishing new version of spring-cloud-gcp ?

@elefeint
Copy link
Contributor

We'll release in the next 2 weeks. Need to get Spring Boot 2.7 compatibility in.

kateryna216 added a commit to kateryna216/spring-cloud-gcp that referenced this pull request Oct 20, 2022
…rs (GoogleCloudPlatform#1152)

* Allow to customize thread name prefixes for subscription specific TaskSchedulers
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.

None yet

2 participants