Hi Oliver,

> Gesendet: Montag, 09. Dezember 2013 um 22:22 Uhr
> Von: "Oliver Wulff" <owu...@talend.com>
> 
> @Guido, is the property "connectionProperties" sufficient which is a 
> semi-colon separated string:
> http://commons.apache.org/proper/commons-dbcp/configuration.html

No, connectionProperties only sets properties on the JDBC driver, not on the 
connection pool.

If the persistence.properties file is considered to be too polluted with too 
many connection pool
settings, I see two possibilities:

* only make the most important ones configurable externally (I've suggested 
some below);
  people who want to change more settings can still overwrite 
persistenceContext.xml; and/or
* use the Spring default mechanism for properties 
(i.e.${jpa.pool.maxActive:8}); thus the properties that keep their default 
value can be omitted in the properties file, with the disadvantage that one 
does not immediately see that they are configurable, and the advantage that 
there is no need to change the configuration file of existing Syncope 
installations. I'd would be even better if one could make Spring not set the 
value at all (i.e. use DBCP's default) if not specified, but I don't think this 
is possible.

WDYT?

@Francesco: I'd be interested in why ISOLATION_READ_COMMITTED was chosen to
be set explicitly as a transaction level (but only on localDataSource, not 
jndi) -
was there a specific reason?

BTW, one can also set the default transaction isolation on dbcp directly (but 
then either as an int value or with some Spring
tricks to refer to a constant). This may be more robust; in the 
IsolationLevelDataSourceAdapter javadoc 
there's a warning that one must make sure that the "target DataSource properly 
cleans up such transaction state"
(though no idea if this applies in our case).

Regards,
   Guido


> @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[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[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][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][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][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][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]]

Reply via email to