@Guido, is the property "connectionProperties" sufficient which is a semi-colon separated string: http://commons.apache.org/proper/commons-dbcp/configuration.html
@Francesco: I understand your concern. On the other hand you could argue that the jpa.properties file becomes completely BasicDataSource dependent. But maybe the connectionProperties property is sufficient. Thanks Oli ________________________________________ From: Francesco Chicchiriccò [ilgro...@apache.org] Sent: 09 December 2013 09:29 To: dev@syncope.apache.org Subject: Re: BasicDataSource usage in persistence.xml On 08/12/2013 14:54, Oliver Wulff wrote: > Hi there > > JIRA raised and patch applied: > https://issues.apache.org/jira/browse/SYNCOPE-460 > > WRT the BasicDataSource properties... what do you think about adding only > connectionProperties which could be a file with the BasicDataSource > properties. Patch looks fine, thanks! I would avoid proliferating of properties file that would make harder times with generation from archetype, so I'd keep with adding entries in persistence.properties. I will wait till we come to an agreement before applying your patch. Regards. > ________________________________________ > From: Francesco Chicchiriccò [ilgro...@apache.org] > Sent: 06 December 2013 16:21 > To: dev@syncope.apache.org > Subject: Re: BasicDataSource usage in persistence.xml > > On 06/12/2013 16:17, Guido Wimmel wrote: >> Hi, >> >> I wouldn't keep Oliver from providing a patch wrt. his original request, >> if he'd like to ;-) >> Though if not, I can possibly also provide something - maybe next week. >> >> I'm only unsure about which of the connection pool settings exactly should >> be externalized into properties, > I'd say _any_ available property. > >> and about the isolation level setting (maybe one >> could make this configurable as well, with default READ_COMMITTED?). > +1 > > For whoever will take care of this: please > (1) consider *all* persistence.properties found under core/ > (2) include commons-dbcp 1.4 as direct dependency either in root > pom.xml (<dependencyManagement/>) and core/pom.xml. > > Regards. > >> Gesendet: Freitag, 06. Dezember 2013 um 15:38 Uhr >> Von: "Oliver Wulff" <owu...@talend.com> >> An: "dev@syncope.apache.org" <dev@syncope.apache.org> >> Betreff: RE: BasicDataSource usage in persistence.xml >> Will do ;-) >> >> Cheers >> Oli >> ________________________________________ >> From: Francesco Chicchiriccò [ilgro...@apache.org] >> Sent: 06 December 2013 13:22 >> To: dev@syncope.apache.org >> Subject: Re: BasicDataSource usage in persistence.xml >> >> On 06/12/2013 12:49, Guido Wimmel wrote: >>> Hi Oliver, >>> >>> FYI: we use such a setup (with dbcp.BasicDataSource) in our project, thus >>> avoiding the need to >>> configure a data source in the container. This works fine; also with our >>> own unit/integration tests. >>> >>> We also used the standard MySQL isolation level (REPEATABLE_READ), as we >>> had problems with our standard >>> MySQL configuration related to Binary Logs ("InnoDB is limited to >>> row-logging when transaction isolation level >>> is READ_COMMITTED or READ_UNCOMMITTED"). This also works fine, but probably >>> the explicit setting of >>> READ_COMMITTED as a transaction level was chosen on purpose. >>> >>> We externalized some more properties of BasicDataSource (testOnBorrow, >>> testOnReturn, removeAbandoned, >>> initialSize, maxActive, minIdle, removeAbandoned, >>> timeBetweenEvictionRunsMillis, minEvictableIdleTimeMillis, >>> validationQuery). IMO, at least the size parameters (initialSize, >>> maxActive, minIdle) and the validation query >>> (which can be database specific) should be configurable via properties. >>> BasicDataSource should also probably be a direct >>> Maven dependency then. >>> >>> I'd be +1 for this change, as it would spare us the need of overwriting >>> persistenceContext.xml ;-) >> Well, what are you still waiting for providing a patch, then? ;-) >> >>>> Von: "Francesco Chicchiriccò" <ilgro...@apache.org> >>>>> Hi there >>>> Hi Oliver, >>>>> Wouldn't it make sense to use a BasicDataSource instead of >>>>> DriverManagerDataSource as it is also recommended from Spring: >>>>> http://docs.spring.io/spring/docs/current/spring-framework-reference/html/jdbc.html#jdbc-connections >>>> Short answer: Yes, why not? >>>> Long answer follows. >>>> >>>> The localDataSource bean in persistenceContext.xml is there for >>>> providing a DataSource for non-production usage: unit and integration >>>> tests, various development profiles [1], embedded mode [2] and >>>> standalone distribution [3]. >>>> >>>> As you can see from syncopeContext.xml, things are arranged so that at >>>> first a JNDI lookup for a proper DataSource is performed: this becomes >>>> of fundamental importance in production environments, as we suggest [4]. >>>> >>>> Moreover, usage of BasicDataSource implies commons-dbcp dependency, >>>> which is currently indirectly pulled in by Activiti >>>> (org.activiti:activiti-spring, exactly). >>>> >>>> Hence, I am +-0 for this change. >>>> >>>>> Instead of this: >>>>> >>>>> <bean id="localDataSource" >>>>> class="org.springframework.jdbc.datasource.IsolationLevelDataSourceAdapter"> >>>>> <property name="targetDataSource"> >>>>> <bean class="org.springframework.jdbc.datasource.DriverManagerDataSource"> >>>>> <property name="driverClassName" value="${jpa.driverClassName}"/> >>>>> <property name="url" value="${jpa.url}"/> >>>>> <property name="username" value="${jpa.username}"/> >>>>> <property name="password" value="${jpa.password}"/> >>>>> </bean> >>>>> </property> >>>>> <property name="isolationLevelName" value="ISOLATION_READ_COMMITTED"/> >>>>> </bean> >>>>> >>>>> >>>>> we can configure this bean: >>>>> >>>>> <bean id="localDataSource" >>>>> class="org.springframework.jdbc.datasource.IsolationLevelDataSourceAdapter"> >>>>> <property name="targetDataSource"> >>>>> <bean class="org.apache.commons.dbcp.BasicDataSource" >>>>> destroy-method="close"> >>>>> <property name="driverClassName" value="${jpa.driverClassName}"/> >>>>> <property name="url" value="${jpa.url}"/> >>>>> <property name="username" value="${jpa.username}"/> >>>>> <property name="password" value="${jpa.password}"/> >>>>> </bean> >>>>> </property> >>>>> <property name="isolationLevelName" value="ISOLATION_READ_COMMITTED"/> >>>>> </bean> >>>> If you like such change, please file an issue and attach a patch there. >>>> >>>> Regards. >>>> >>>> [1] >>>> http://syncope.apache.org/building.html#More_build_profiles[http://syncope.apache.org/building.html#More_build_profiles] >>>> [2] >>>> https://cwiki.apache.org/confluence/display/SYNCOPE/Run+Syncope+in+embedded+mode[https://cwiki.apache.org/confluence/display/SYNCOPE/Run+Syncope+in+embedded+mode] >>>> [3] >>>> https://cwiki.apache.org/confluence/display/SYNCOPE/Run+Syncope+standalone+distribution[https://cwiki.apache.org/confluence/display/SYNCOPE/Run+Syncope+standalone+distribution] >>>> [4] >>>> https://cwiki.apache.org/confluence/display/SYNCOPE/Run+Syncope+in+real+environments#RunSyncopeinrealenvironments-Usedatasource[https://cwiki.apache.org/confluence/display/SYNCOPE/Run+Syncope+in+real+environments#RunSyncopeinrealenvironments-Usedatasource] -- Francesco Chicchiriccò Tirasa - Open Source Excellence http://www.tirasa.net/ ASF Member, Apache Syncope PMC chair, Apache Cocoon PMC Member http://people.apache.org/~ilgrosso/
