On 10/12/2013 08:57, Guido Wimmel wrote:
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?
Hi Guido,
I'd say latter option (e.g. Spring properties with default values) is
fine: would you mind to grab Oliver's patch, apply your changes and
re-attach?
@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?
A quick search of mail archives (via http://syncope.markmail.org) gave
this commit (released as part of 1.0.2-incubating)
http://svn.apache.org/viewvc?view=revision&revision=1392392
which is bound to
https://issues.apache.org/jira/browse/SYNCOPE-124
https://issues.apache.org/jira/browse/SYNCOPE-202
SYNCOPE-202 in particular refers to integration tests problems with
MySQL (and Oracle): here's why the isolation level was set to the
DataSource that was supposed - till this discussion - to be only used
for integration tests.
This fact also leads to the consideration that before committing any
patch, all supported DBMS needs to be checked.
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).
Hum, not sure either of what that means: anyway in general I'd say DBCP
is good enough for cleaning up its stuff.
Regards.
@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]]
--
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/
