Skip to content

Conversation

kazuki43zoo
Copy link
Contributor

@kazuki43zoo kazuki43zoo commented Dec 3, 2016

I will submit a feature request that support externalized properties for spring transaction manager. In this request, following properties are supported.

  • defaultTimeout (This setting is useful to prevent a long running transaction by default)
  • rollbackOnCommitFailure (e.g. when it use on the Oracle, this setting should be set to the true because the Oracle perform implicit commit when close a connection)

e.g. )

spring.transaction.default-timeout=60
spring.transaction.rollback-on-commit-failure=true

What do you think about this request ?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Dec 3, 2016
@kazuki43zoo kazuki43zoo force-pushed the support-externalized-properties-on-tx branch from 7f25105 to 68b7274 Compare December 3, 2016 08:19
@vpavic
Copy link
Contributor

vpavic commented Dec 3, 2016

👍 for this proposal.

However I think it should also affect Neo4jDataAutoConfiguration since it configures Neo4jTransactionManager.

* default-timeout
* rollbac-on-commit-failure
@kazuki43zoo kazuki43zoo force-pushed the support-externalized-properties-on-tx branch from 68b7274 to e8e032f Compare December 3, 2016 09:51
@kazuki43zoo
Copy link
Contributor Author

Hi @vpavic , thanks for your comment !
I've fixed your comment via https://github.com/spring-projects/spring-boot/pull/7561/files#diff-0e70354246fa64a47fb17cfb2a6a6137.

@philwebb philwebb added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 5, 2016
@philwebb philwebb added this to the 1.5.0 RC1 milestone Dec 5, 2016
philwebb pushed a commit that referenced this pull request Dec 21, 2016
Add Spring TransactionManager properties to allow timeout and rollback
settings to be configured.

See gh-7561
philwebb added a commit that referenced this pull request Dec 21, 2016
…ties-on-tx

* pr/7561:
  Polish spring transaction manager properties
  Support spring transaction manager properties
@philwebb philwebb closed this in 99e7266 Dec 21, 2016
@philwebb
Copy link
Member

@kazuki43zoo Thanks for the PR. I've reworked things a bit in commit 99e7266 if you'd like to review. The main change that I've made is to make TrasnsactionProperties a nested configuration. This allows us separate transaction manager configuration per technology if required and also means we don't need to inject the extra properties class.

Thanks again for the contribution!

@snicoll
Copy link
Member

snicoll commented Dec 21, 2016

There is no need for transaction.* in the doc, we should list all the properties. Also, I am not sure I like spring.batch.transaction.* in light of #7259 - IMO, it is weird to have different settings in the same app: a transaction manager is a central thing so offering several ways of configuring it is misleading.

@snicoll snicoll reopened this Dec 21, 2016
@snicoll snicoll added for: team-attention An issue we'd like other members of the team to review and removed for: team-attention An issue we'd like other members of the team to review labels Dec 27, 2016
@philwebb philwebb self-assigned this Dec 28, 2016
philwebb added a commit that referenced this pull request Dec 29, 2016
Add a `TransactionManagerCustomizer` callback interface that can be
used to customize auto-configured `PlatformTransactionManagers`.

Also update `...transaction.*` properties under a single unified
`spring.transaction...` key since the existing auto-configurations
would often share a transaction manager (the technology specific
transaction managers are `@ConditionalOnMissingBean` and may use
a manager created by a previous auto-configuration).

See gh-7561
@philwebb
Copy link
Member

@snicoll Could you review f22744c if you get a chance and see if it makes more sense?

@snicoll snicoll closed this in 4b641ca Dec 29, 2016
@snicoll
Copy link
Member

snicoll commented Dec 29, 2016

@philwebb looks sensible. I've polished minor things in 4b641ca. The only thing that bugs me at this point is that TransactionAutoConfiguration provides the TransactionManagerCustomizers bean and should run after 3 other auto-configurations and these are actually relying on that bean. It works at runtime because the way that dependency is injected but conceptually it is a bit weird.

Not sure that we need to act on it though because that would make things a bit more complicated.

@kazuki43zoo
Copy link
Contributor Author

Hi @philwebb and @snicoll, thank you for great refactoring !!

@kazuki43zoo kazuki43zoo deleted the support-externalized-properties-on-tx branch December 29, 2016 11:29
kazuki43zoo added a commit to kazuki43zoo/spring-boot that referenced this pull request Dec 29, 2016
@philwebb
Copy link
Member

@snicoll Yeah the order of auto-configuration bugged me as well. Actually, I should check that it's not caused a package tangle.

@kazuki43zoo
Copy link
Contributor Author

kazuki43zoo commented Dec 29, 2016

Hi @philwebb,

I've tried 1.5.0.BUILD-SNAPSHOT. I found an one issue on creating a customizer using lambda expression as follow:

@Bean
PlatformTransactionManagerCustomizer<JtaTransactionManager> transactionManagerCustomizer() {
	return transactionManager -> {
		System.out.println("jta");
		System.out.println(transactionManager.getDefaultTimeout());
	};
}

Above code is fail at the start up time. I've enabled the DataSourceTransactionManager. Hence; I will expect that customization is skip. However it does not work fine.

...
2016-12-30 02:52:04.871  WARN 53709 --- [           main] s.c.a.AnnotationConfigApplicationContext : Exception encountered during context initialization - cancelling refresh attempt: org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'transactionManager' defined in class path resource [org/springframework/boot/autoconfigure/jdbc/DataSourceTransactionManagerAutoConfiguration$DataSourceTransactionManagerConfiguration.class]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.jdbc.datasource.DataSourceTransactionManager]: Factory method 'transactionManager' threw exception; nested exception is java.lang.ClassCastException: org.springframework.jdbc.datasource.DataSourceTransactionManager cannot be cast to org.springframework.transaction.jta.JtaTransactionManager
2016-12-30 02:52:04.871  INFO 53709 --- [           main] o.s.j.e.a.AnnotationMBeanExporter        : Unregistering JMX-exposed beans on shutdown
2016-12-30 02:52:04.877  INFO 53709 --- [           main] utoConfigurationReportLoggingInitializer : 

Error starting ApplicationContext. To display the auto-configuration report re-run your application with 'debug' enabled.
2016-12-30 02:52:04.888 ERROR 53709 --- [           main] Disconnected from the target VM, address: '127.0.0.1:51099', transport: 'socket'
o.s.boot.SpringApplication               : Application startup failed

org.springframework.beans.factory.BeanCreationException: Error creating bean with name 'transactionManager' defined in class path resource [org/springframework/boot/autoconfigure/jdbc/DataSourceTransactionManagerAutoConfiguration$DataSourceTransactionManagerConfiguration.class]: Bean instantiation via factory method failed; nested exception is org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.jdbc.datasource.DataSourceTransactionManager]: Factory method 'transactionManager' threw exception; nested exception is java.lang.ClassCastException: org.springframework.jdbc.datasource.DataSourceTransactionManager cannot be cast to org.springframework.transaction.jta.JtaTransactionManager
	at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:599) ~[spring-beans-4.3.5.RELEASE.jar:4.3.5.RELEASE]
	at org.springframework.beans.factory.support.AbstractAutowireCapableBeanFactory.instantiateUsingFactoryMethod(AbstractAutowireCapableBeanFactory.java:1134) ~[spring-beans-4.3.5.RELEASE.jar:4.3.5.RELEASE]
...
	... 17 common frames omitted
Caused by: java.lang.ClassCastException: org.springframework.jdbc.datasource.DataSourceTransactionManager cannot be cast to org.springframework.transaction.jta.JtaTransactionManager
	at org.springframework.boot.autoconfigure.transaction.TransactionManagerCustomizers.customize(TransactionManagerCustomizers.java:59) ~[spring-boot-autoconfigure-1.5.0.BUILD-20161229.095255-265.jar:1.5.0.BUILD-SNAPSHOT]
...

As alternative, I've created a customizer class instead of lambda expression as follow:

@Component
static class MyJtaTransactionManagerCustomizer implements PlatformTransactionManagerCustomizer<JtaTransactionManager> {
	@Override
	public void customize(JtaTransactionManager transactionManager) {
		System.out.println("jta");
		System.out.println(transactionManager.getDefaultTimeout());
	}
}

It work fine (it can be skip a customization).

Is this behavior work as design ? or bug ? or limitation ?

@philwebb
Copy link
Member

philwebb commented Dec 30, 2016

@kazuki43zoo This is an unfortunate limitation of using a lambda See SPR-12525 for some discussion. We might, however, be able to use a similar trick to that discussed in SPR-14109.

@kazuki43zoo
Copy link
Contributor Author

@philwebb Thanks for good information !!

philwebb pushed a commit that referenced this pull request Dec 30, 2016
Remove a few nested `TransactionProperties` that should have been
deleted in commit f22744c.

See gh-7561
Closes gh-7786
philwebb referenced this pull request Dec 30, 2016
* pr/7786:
  Remove a nested TransactionProperties
philwebb added a commit that referenced this pull request Dec 30, 2016
Update `TransactionManagerCustomizers` to deal directly with
`ClassCastExceptions` assuming that they are because a customizer is
implemented using a lambda.

See gh-7561
kazuki43zoo added a commit to kazuki43zoo/spring-boot that referenced this pull request Dec 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants