You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by Patrick Linskey <pl...@bea.com> on 2007/04/06 18:08:54 UTC
RE: svn commit: r525997 - in /incubator/openjpa/trunk: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/ openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main
> Why is this setting called "IsolationLevel" where our global setting
> is called "TransactionIsolation"? Shouldn't this local setting just
> be called "Isolation" for consistency? Same with the
> FetchPlan facade.
Personally, I feel that 'IsolationLevel' is a more-well-known term for
the concept. I'm fine with either name, though.
--
Patrick Linskey
BEA Systems, Inc.
_______________________________________________________________________
Notice: This email message, together with any attachments, may contain
information of BEA Systems, Inc., its subsidiaries and affiliated
entities, that may be confidential, proprietary, copyrighted and/or
legally privileged, and is intended solely for the use of the individual
or entity named in this message. If you are not the intended recipient,
and have received this message in error, please immediately return this
by email and then delete it.
> -----Original Message-----
> From: Abe White [mailto:awhite@bea.com]
> Sent: Friday, April 06, 2007 7:10 AM
> To: open-jpa-dev@incubator.apache.org
> Subject: Re: svn commit: r525997 - in
> /incubator/openjpa/trunk:
> openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/
> openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/
> openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel
> / openjpa-jdbc/src/main
>
> Comments inline...
>
> > + /**
> > + * <p>The isolation level for queries issued to the database.
> > This overrides
> > + * the persistence-unit-wide
> > <code>openjpa.jdbc.TransactionIsolation</code>
> > + * value.</p>
> > + *
> > + * <p>Must be one of {@link Connection#TRANSACTION_NONE},
> > + * {@link Connection#TRANSACTION_READ_UNCOMMITTED},
> > + * {@link Connection#TRANSACTION_READ_COMMITTED},
> > + * {@link Connection#TRANSACTION_REPEATABLE_READ},
> > + * {@link Connection#TRANSACTION_SERIALIZABLE},
> > + * or -1 for the default connection level specified by the
> > context in
> > + * which this fetch configuration is being used.</p>
> > + *
> > + * @since 0.9.7
> > + */
> > + public int getIsolationLevel();
>
> Why is this setting called "IsolationLevel" where our global setting
> is called "TransactionIsolation"? Shouldn't this local setting just
> be called "Isolation" for consistency? Same with the
> FetchPlan facade.
>
> >
> > public Set joins = null;
> > + public int isolationLevel = -1;
>
> The FetchConfiguration interface defines a DEFAULT constant.
> The doc
> on this constant is: "Constant to revert any setting back to its
> default value." We should either change all the places where we're
> using -1 to use DEFAULT or at least change the setIsolation
> setter to
> look for DEFAULT and translate it to -1.
I chose -1 to parallel the JavaDoc and code in JDBCConfiguration /
JDBCConfigurationImpl -- there, -1 is used for the 'default' alias.
> at least change the setIsolation setter to
> look for DEFAULT and translate it to -1.
Done.
> > + public JDBCFetchConfiguration setIsolationLevel(int level) {
> > + if (level != -1
> > + && level != Connection.TRANSACTION_NONE
> > + && level != Connection.TRANSACTION_READ_UNCOMMITTED
> > + && level != Connection.TRANSACTION_READ_COMMITTED
> > + && level != Connection.TRANSACTION_REPEATABLE_READ
> > + && level != Connection.TRANSACTION_SERIALIZABLE)
> > + throw new IllegalArgumentException(
> > + _loc.get("bad-level", Integer.valueOf
> > (level)).getMessage());
>
> Switch statement...
I prefer if statements, and know of no reason to use a switch instead.
> > + /**
> > + * Get the update clause for the query based on the
> > + * updateClause and isolationLevel hints
> > + */
> > + protected String
> getForUpdateClause(JDBCFetchConfiguration fetch,
> > + boolean forUpdate) {
> > + if (fetch.getIsolationLevel() != -1)
> > + throw new IllegalStateException(_loc.get(
> > + "isolation-level-config-not-supported", getClass
> > ().getName())
> > + .getMessage());
> > + else
> > + return forUpdateClause;
> > }
>
> Any reason we aren't using our own InvalidStateException (extends
> UserException)?
Are there any advantages to doing so? Now that exceptions nest well and
we have exception translation at the boundaries, it seems as though
there's no reason to use our internal exception types vs. standard Java
exceptions like IllegalStateException or IllegalArgumentException. I can
go either way on this though.
> > if (forUpdate && !simulateLocking) {
> > assertSupport(supportsSelectForUpdate,
> > "SupportsSelectForUpdate");
> > - if (forUpdateClause != null)
> > - buf.append(" ").append(forUpdateClause);
> > + if (this.forUpdateClause != null)
> > + buf.append(" ").append(this.forUpdateClause);
> > }
>
> Why are we passing a new "forUpdateClause" param to this method if
> we're going to ignore it and use our "forUpdateClause" member?
Bug. I'm not sure how that got in there. Bummer that we don't have unit
tests for any of the DB2-specific behavior. Fixed in 526212.
-Patrick
Notice: This email message, together with any attachments, may contain information of BEA Systems, Inc., its subsidiaries and affiliated entities, that may be confidential, proprietary, copyrighted and/or legally privileged, and is intended solely for the use of the individual or entity named in this message. If you are not the intended recipient, and have received this message in error, please immediately return this by email and then delete it.
RE: svn commit: r525997 - in /incubator/openjpa/trunk: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/ openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main
Posted by Patrick Linskey <pl...@bea.com>.
-1 to anything with 'transaction' in it. This is a per-query override
for the transaction isolation level, not a per-transaction setting.
-Patrick
--
Patrick Linskey
BEA Systems, Inc.
_______________________________________________________________________
Notice: This email message, together with any attachments, may contain
information of BEA Systems, Inc., its subsidiaries and affiliated
entities, that may be confidential, proprietary, copyrighted and/or
legally privileged, and is intended solely for the use of the individual
or entity named in this message. If you are not the intended recipient,
and have received this message in error, please immediately return this
by email and then delete it.
> -----Original Message-----
> From: Marc Prud'hommeaux [mailto:mprudhomapache@gmail.com] On
> Behalf Of Marc Prud'hommeaux
> Sent: Friday, April 06, 2007 12:48 PM
> To: open-jpa-dev@incubator.apache.org
> Subject: Re: svn commit: r525997 - in
> /incubator/openjpa/trunk:
> openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/
> openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/
> openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel
> / openjpa-jdbc/src/main
>
>
> FWIW, my personal naming preference in descending order:
>
> 1. setTransactionIsolationLevel
> 2. setTransactionIsolation (user: "Is this a boolean? Of course I
> want my transactions to be isolated!")
> 3. setIsolation (user: "What exactly is being 'isolated'? The
> EntityManager or the cache or something?")
> 4. setIsolationLevel (same a #3)
>
> I'd really be fine with any of them, though.
>
>
> On Apr 6, 2007, at 12:31 PM, Abe White wrote:
>
> >>> Why is this setting called "IsolationLevel" where our
> global setting
> >>> is called "TransactionIsolation"? Shouldn't this local
> setting just
> >>> be called "Isolation" for consistency? Same with the
> >>> FetchPlan facade.
> >>
> >> Personally, I feel that 'IsolationLevel' is a
> more-well-known term
> >> for
> >> the concept. I'm fine with either name, though.
> >
> > Does anyone else have an opinion on this? I think get/setIsolation
> > would be more consistent with the global TransactionIsolation
> > property. I doubt the lack of a "Level" suffix is going to hurt.
> >
> > Notice: This email message, together with any attachments, may
> > contain information of BEA Systems, Inc., its subsidiaries
> > and affiliated entities, that may be confidential,
> proprietary,
> > copyrighted and/or legally privileged, and is intended solely for
> > the use of the individual or entity named in this message. If you
> > are not the intended recipient, and have received this message in
> > error, please immediately return this by email and then delete it.
>
>
Notice: This email message, together with any attachments, may contain information of BEA Systems, Inc., its subsidiaries and affiliated entities, that may be confidential, proprietary, copyrighted and/or legally privileged, and is intended solely for the use of the individual or entity named in this message. If you are not the intended recipient, and have received this message in error, please immediately return this by email and then delete it.
Re: svn commit: r525997 - in /incubator/openjpa/trunk: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/ openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main
Posted by Marc Prud'hommeaux <mp...@apache.org>.
FWIW, my personal naming preference in descending order:
1. setTransactionIsolationLevel
2. setTransactionIsolation (user: "Is this a boolean? Of course I
want my transactions to be isolated!")
3. setIsolation (user: "What exactly is being 'isolated'? The
EntityManager or the cache or something?")
4. setIsolationLevel (same a #3)
I'd really be fine with any of them, though.
On Apr 6, 2007, at 12:31 PM, Abe White wrote:
>>> Why is this setting called "IsolationLevel" where our global setting
>>> is called "TransactionIsolation"? Shouldn't this local setting just
>>> be called "Isolation" for consistency? Same with the
>>> FetchPlan facade.
>>
>> Personally, I feel that 'IsolationLevel' is a more-well-known term
>> for
>> the concept. I'm fine with either name, though.
>
> Does anyone else have an opinion on this? I think get/setIsolation
> would be more consistent with the global TransactionIsolation
> property. I doubt the lack of a "Level" suffix is going to hurt.
>
> Notice: This email message, together with any attachments, may
> contain information of BEA Systems, Inc., its subsidiaries
> and affiliated entities, that may be confidential, proprietary,
> copyrighted and/or legally privileged, and is intended solely for
> the use of the individual or entity named in this message. If you
> are not the intended recipient, and have received this message in
> error, please immediately return this by email and then delete it.
Re: svn commit: r525997 - in /incubator/openjpa/trunk: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/ openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/kernel/ openjpa-jdbc/src/main
Posted by Abe White <aw...@bea.com>.
>> Why is this setting called "IsolationLevel" where our global setting
>> is called "TransactionIsolation"? Shouldn't this local setting just
>> be called "Isolation" for consistency? Same with the
>> FetchPlan facade.
>
> Personally, I feel that 'IsolationLevel' is a more-well-known term for
> the concept. I'm fine with either name, though.
Does anyone else have an opinion on this? I think get/setIsolation
would be more consistent with the global TransactionIsolation
property. I doubt the lack of a "Level" suffix is going to hurt.
Notice: This email message, together with any attachments, may contain information of BEA Systems, Inc., its subsidiaries and affiliated entities, that may be confidential, proprietary, copyrighted and/or legally privileged, and is intended solely for the use of the individual or entity named in this message. If you are not the intended recipient, and have received this message in error, please immediately return this by email and then delete it.