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

Add an option to start the containers after the ApplicationContext is refreshed #723

Open
jacobmarshmallow opened this issue Mar 21, 2023 · 14 comments
Labels
component: sqs SQS integration related issue status: ideal-for-contribution We agree it's nice to have but it is not team priority type: enhancement Smaller enhancement in existing integration
Milestone

Comments

@jacobmarshmallow
Copy link

Type: Bug

Component:
SQS

Describe the bug
I'm unsure if this is intended but thought I'd raise it just in case. when using spring.main.lazy-initialization=true our listeners marked with @SqsListener don't get registered unless marked with @Lazy(false). It's not a huge issue but SQS is one of the main reasons for slow service start times and isn't necessary immediately in this service.

Versions:

spring-boot: 3.0.4
spring-cloud-dependencies: 2022.0.1
spring-cloud-aws-dependencies: 3.0.0-RC1

Sample
I can provide a smallest possible reproduction if needed, but it was essentially an empty project with spring boot 3 web + cloud + messaging + sqs

@maciejwalkowiak maciejwalkowiak added component: sqs SQS integration related issue type: bug Something isn't working labels Mar 25, 2023
@maciejwalkowiak maciejwalkowiak added this to the 3.x milestone Mar 25, 2023
@tomazfernandes
Copy link
Contributor

tomazfernandes commented Mar 26, 2023

Hey @jacobmarshmallow, thanks for bringing this up.

Did this work with Spring Cloud AWS 2.x?

Just FYI, in Spring Cloud AWS 3.0 containers created via @SqsListener are started in parallel by default, so hopefully this won't be the bottleneck for application startup anymore.

Sample
I can provide a smallest possible reproduction if needed, but it was essentially an empty project with spring boot 3 web + cloud + messaging + sqs

That would actually be helpful, thanks.

@jacobmarshmallow
Copy link
Author

Hi @tomazfernandes

Did this work with Spring Cloud AWS 2.x?

I'm not actually sure if it worked with 2.x (and happy for it to be a feature request / noted in the docs).

... hopefully this won't be the bottleneck for application startup anymore.

That's good to know, I think in this case it's just one listener being set up and it takes over 10s to make the first connection (to get the queue URL) so we were able to inject that directly as a property. We also tried the CRT client for faster start times but it didn't work either (and is still pre v1).

Here is a replication using test containers, but it was originally discovered in ECS.

@tomazfernandes
Copy link
Contributor

I'm not actually sure if it worked with 2.x (and happy for it to be a feature request / noted in the docs).

Yeah, at this point I'm prioritizing missing features from the previous version getting in the way of people migrating to 3.0. But I should get to it in time for 3.0 nevertheless.

That's good to know, I think in this case it's just one listener being set up and it takes over 10s to make the first connection (to get the queue URL) so we were able to inject that directly as a property. We also tried the CRT client for faster start times but it didn't work either (and is still pre v1).

Is the SQS startup still slow after you feed it the queue URL? And why does it take 10s to get the queue URL, is this expected?

Here is a replication using test containers, but it was originally discovered in ECS.

Thanks, I'll take a look. It seems though you're mixing up Spring Cloud AWS versions in your pom.xml:

<dependency>
	<groupId>io.awspring.cloud</groupId>
	<artifactId>spring-cloud-aws-sqs</artifactId>
</dependency>
<dependency>
	<groupId>io.awspring.cloud</groupId>
	<artifactId>spring-cloud-starter-aws-messaging</artifactId>
	<version>2.4.4</version>
</dependency>

Would you mind replacing these for the 3.0-RC1 SQS starter and checking if the behavior persists?

Thanks

@jacobmarshmallow
Copy link
Author

Is the SQS startup still slow after you feed it the queue URL?

No, it's fine now as it doesn't need to make any requests

And why does it take 10s to get the queue URL, is this expected?

I'm not sure but we have experienced slow first requests using dynamo before, 10 seconds to get a URL is quite excessive though.

It seems though you're mixing up Spring Cloud AWS versions in your pom.xml:

Thanks, I've fixed it and updated it to use RC2 now 🙂. The problem of lazy listeners not getting initialized still persists

@maciejwalkowiak
Copy link
Contributor

I'm wondering how is it with Kafka, AMQP or other Spring modules.

@tomazfernandes
Copy link
Contributor

I'll try to take a look into this this weekend.

@tomazfernandes
Copy link
Contributor

@jacobmarshmallow the way @SqsListener works is we have a BeanPostProcessor that detects the annotation on a bean and bootstraps the container instantiation. With your beans set to lazy, nothing triggers the instantiation of the bean and hence it doesn't work.

What we can look into adding would be implementing SmartLifecycle#isAutoStartup, then you can set it to false and start the containers manually when necessary.

How does this sound?

@jacobmarshmallow
Copy link
Author

Hey @tomazfernandes that sounds like a good compromise

@tomazfernandes
Copy link
Contributor

@jacobmarshmallow just curious - what’s the behavior you’d expect from using the listener beans as Lazy?

@tomazfernandes tomazfernandes added type: enhancement Smaller enhancement in existing integration and removed type: bug Something isn't working labels Apr 4, 2023
@jacobmarshmallow
Copy link
Author

@tomazfernandes it's a fair question, I was hoping that the listener bean would still be initialised and process SQS messages just that it wouldn't block the Tomcats start up. So that the REST requests can start being processed ASAP and the secondary functionality of processing the SQS messages would still occur but can be slower

@tomazfernandes
Copy link
Contributor

Hmm, that's an interesting proposition.

I think we could have an option of registering the beans at startup with autoStartup set to false, and then listen to an ApplicationEvent to spin the containers up after everything is setup. How does that sound?

The downside would be that if an error occurs when spinning up the containers we would need to decide whether to stop the application or not, but either way the application would already be serving requests.

I think we have two separate features here - implement auto startup, and then look into adding an option of starting the containers after the application is up. Would you like to open these issues?

Thanks

@tomazfernandes
Copy link
Contributor

Hey @jacobmarshmallow, I wonder if you'd be interested in contributing a PR with this feature.

Thanks.

@tomazfernandes
Copy link
Contributor

tomazfernandes commented Jun 28, 2023

We now have autoStartup in place (#827), so for this feature we'd need to have an option to create all listener from SqsListener as autoStartup false, and in the DefaultListenerContainerRegistry have some logic to start the containers after the ApplicationContext is refreshed.

@tomazfernandes tomazfernandes changed the title Support spring boot lazy initialization Add an option to start the containers after the ApplicationContext is refreshed Jun 28, 2023
@tomazfernandes tomazfernandes added the status: ideal-for-contribution We agree it's nice to have but it is not team priority label Jun 28, 2023
@laszlosebok
Copy link

Just chipping in so others with similar problems may find this issue.

We have upgraded our application to Spring Boot 3.2.0 from Spring Boot 2.x recently and the @SqsListener annotation would stop working, even though all the correct dependencies have been upgraded.

At the end of the day, the issue is the same: We have the @RefreshScope annotation on our class which contains our @SqsListener methods. The @RefreshScope annotation implicitly makes it lazy loaded and when BeanPostProcessor afterSingletonsInstantiated runs, endpoints are not yet registered by detectAnnotationsAndRegisterEndpoints

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: sqs SQS integration related issue status: ideal-for-contribution We agree it's nice to have but it is not team priority type: enhancement Smaller enhancement in existing integration
Projects
None yet
Development

No branches or pull requests

4 participants