openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Patrick Linskey" <plins...@bea.com>
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
Date Fri, 06 Apr 2007 16:08:54 GMT
> 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.

Mime
View raw message