-
-
Notifications
You must be signed in to change notification settings - Fork 353
#1457-Introduce-kinesis-autoconfiguration #1483
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
base: main
Are you sure you want to change the base?
#1457-Introduce-kinesis-autoconfiguration #1483
Conversation
maciejwalkowiak
left a comment
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.
Assuming it is compatible with #1479 we can merge!
But if we are adding Kinesis integration we can also think about:
artembilan
left a comment
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.
Just couple hygiene nit-picks.
This affects my PR for spring-cloud-aws-kinesis module only at the dependency .
The KPL/KCL could be auto-configured in other PRs.
Thanks
...gure/src/main/java/io/awspring/cloud/autoconfigure/kinesis/KinesisAsyncClientCustomizer.java
Outdated
Show resolved
Hide resolved
|
Hey @artembilan I will add KPL/KCL auto configuration after I merge yours PR. Will then align dependencies and other stuff. |
...onfigure/src/main/java/io/awspring/cloud/autoconfigure/kinesis/KinesisAutoConfiguration.java
Show resolved
Hide resolved
spring-cloud-aws-starters/spring-cloud-aws-starter-kinesis/pom.xml
Outdated
Show resolved
Hide resolved
...main/java/io/awspring/cloud/autoconfigure/kinesis/KinesisClientLibraryAutoConfiguration.java
Outdated
Show resolved
Hide resolved
artembilan
left a comment
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.
Looks great so far!
|
|
||
| @ConditionalOnMissingBean | ||
| @Bean | ||
| public Scheduler scheduler(ObjectProvider<DynamoDbAsyncClient> dynamoDbClient, |
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.
This is an active components which requires some lifecycle control.
Plus you cannot guess if end-user is interested only in one of this.
We may go something like SchedulerFactory to hide some common properties, but the rest should be left up to end-user configuration.
This about this like the listener container behind @SqsListener.
And I believe eventually we can come up with something like @KclListener
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 had a same idea for 4.1 to introduce KclListener and work on it! :)
Edit:
Actually thinking about it I will take little bit longer do dig into SqsContainer and KafkaContainer and will do it as part od 4.0.0
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 will take some time reading through Spring Messaging, Spring Kafka and SQS integration. I need more reading on this, but i understand the point behind SchedularFactory and containers. Tnx on constructive feedback Artem, I appreciate it much.
| @ConditionalOnClass({ KinesisAsyncClient.class, Scheduler.class }) | ||
| @EnableConfigurationProperties({ KinesisClientLibraryProperties.class }) | ||
| @AutoConfigureAfter({ CredentialsProviderAutoConfiguration.class, RegionProviderAutoConfiguration.class, | ||
| KinesisAutoConfiguration.class }) |
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.
Cannot we use CloudWatchExportAutoConfiguration instead of ObjectProvider?
I'm not sure if DynamoDbAsyncClient is the goal in this project yet, though...
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.
Will move away from ObjectProvider since both DynamoDbAsyncClient and CloudWatchAsync client are not nullabe for Scheduler.
Meaning will have to play little bit with async autoconfiguration in general since most of our clients are sync
📢 Type of change
📜 Description
💡 Motivation and Context
💚 How did you test it?
📝 Checklist
🔮 Next steps