-
-
Notifications
You must be signed in to change notification settings - Fork 196
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
Spring boot support #63
Conversation
This enables creation of different modules more easily
This needs to be verified
This should be named ${project}-spring-boot-starter by convention
See https://editorconfig.org for more information
Awesome, thanks! Will have a look! :) |
Please feel free to be frank in the review. I know Spring Boot better than the DB scheduler :-)
… 23. jul. 2019 kl. 20:46 skrev Gustav Karlsson ***@***.***>:
Awesome, thanks! Will have a look! :)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
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 good, though probably will need a second scan of things :)
Would be neat with some short example Main
showing a minimal example, ala for example https://github.com/kagkarlsson/db-scheduler/blob/master/src/test/java/com/github/kagkarlsson/scheduler/example/CronMain.java
Spring Expression Language (SpEL) under Apache License, Version 2.0 | ||
Spring JDBC under Apache License, Version 2.0 | ||
Spring TestContext Framework under Apache License, Version 2.0 | ||
Spring Transaction under Apache License, Version 2.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.
Will check this later..
...n/java/com/github/kagkarlsson/scheduler/boot/autoconfigure/DbSchedulerAutoConfiguration.java
Outdated
Show resolved
Hide resolved
...tarter/src/main/java/com/github/kagkarlsson/scheduler/boot/config/DbSchedulerProperties.java
Outdated
Show resolved
Hide resolved
...n/java/com/github/kagkarlsson/scheduler/boot/autoconfigure/DbSchedulerAutoConfiguration.java
Outdated
Show resolved
Hide resolved
Hmm, I think we are missing the Tasks that also implement |
I've implemented the As for the examples, should they be separate "real" Spring Boot projects? I was thinking something like (Maven hierarchy):
The benefit of this approach is breakage of the examples if the library/integrations change :-) |
This adds a new getter in Scheduler
log.info("Creating DB Scheduler using tasks from Spring context: {}", configuredTasks); | ||
|
||
if (existingDataSource instanceof TransactionAwareDataSourceProxy) { | ||
log.info("Using a transaction aware DataSource"); |
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 this really necessary information to give? I guess the else block info makes more sense than this?
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.
Probably not worthy of an info
but a debug
might be handy?
if (existingDataSource instanceof TransactionAwareDataSourceProxy) { | ||
log.info("Using a transaction aware DataSource"); | ||
} else { | ||
log.info("The configured DataSource is not transaction aware: '{}'. Operations such as schedule, reschedule and cancel will have effect regardless of spanning transactions.", existingDataSource); |
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 log statement is a bit confusing. What is the purpose of this?
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.
Are there cases where a Spring-user might send in a datasource is not transaction-aware by mistake?
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.
@cash1981: I thought that information whether one uses an instance of TransactionAwareDataSourceProxy
would be useful information, see the docs.
However, I can't seem to get Spring Boot's configuration to provide me with a TransactionAwareDataSourceProxy
out of the box (using JPA or JDBC starter, @EnableTransactionManagement
on/off and using a local PostgreSQL db).
Perhaps also update the documentation on how to add db-scheduler using Spring and this PR? |
Absolutely @cash1981! As pointed out in this comment, we should consider adding some examples and link to those as well. |
@evenh Yeah for spring-boot I think it makes sense to have an examples-directory with a full project that is part of the build. We can link to it from the documentation. If necessary, code can be copied to the markdown-doc. Maybe a separate markdown-page for usage in spring-boot which we link to. If you could create an example project @evenh , I can update the doc :) |
Fixed @kagkarlsson :) |
Ran the example and shutdown seem to be working as exepected @cash1981 |
I think we need to wrap the datasource in a |
Enabled maven-dependency-plugin:
|
Merging this PR. Further improvements can be suggested as separate issues. |
Spring boot support
This PR addresses #55 and provides an initial implementation for Spring Boot integration.