openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Pinaki Poddar" <ppod...@bea.com>
Subject RE: Discussion on proposed change of dynamic modification of Values
Date Wed, 29 Aug 2007 19:55:10 GMT
  
I have gone ahead and made the changes to make the Values themselves be
responsible to judge whether they are 
changeable. Essentially, the Values assert whether they are changeable
in their set() method only if they are dynamic and their parent
configuration is not frozen.

While running the test suite, I am finding out that this strategy *may*
interfere
with how the lazy instantiation of plugin and freezing of BrokerFactory
happens in the current architecture.
BrokerFactory first makes the configuration read-only and then goes
instantiating plugins. Things now starts breaking during plugin
instantiation as 
Value.set() methods think they are being changed within a frozen
Configuration.
Changing the order of these two operations does not solve it too because
some of the
Plugins are laziliy instantiated. 

Why I said is *may* interfere, because this is what I noticed as the
first trial to run the tests.
There may be some obvious way to accommodate the new strategy with the
order of Config freeze and Plugin instantiation.

Any suggestions welcome ...          


Pinaki Poddar
972.834.2865

-----Original Message-----
From: Patrick Linskey [mailto:plinskey@gmail.com] 
Sent: Wednesday, August 29, 2007 11:06 AM
To: dev@openjpa.apache.org
Subject: Re: Discussion on proposed change of dynamic modification of
Values

Hi,

> 5. Make Value.valueChanged() final

Making it final seems a bit over-the-top. If it's not to be used by
subclasses, maybe we should just make it private?

> 1. Rename ValueListener.valueChanged() to valueChanging() to reflect 
> the vetoing power of ValueListener 2. Reorder set() methods in all 
> Value implementations to allow ValueListener to veto and also to 
> enable copy-on-post-freeze-write

So reading through all the logic around valueChanged() (in particular,
the set(foo, true) logic), I don't think I fully grok what's going on.
Maybe we should leave that code alone for now and instead create a new
'boolean allowChange(Object newVal)' method, whose sole purpose is
validating that a change is legal. The default impl can just 'return
false || conf.isDynamic()' (we could pass in a Configuration to the
method, but I think it'd be better to add a Configuration to the
constructors of the Values, since they're inherently associated with a
single Configuration anyways).

Then, we can put an allowChange() call (or maybe an assertChangeable()
call, which just calls allowChange() and throws if it returns false) in
all the branches of set(), setString(), and setObject(). We can also
take a conservative approach and change the existing
assertNotReadOnly() calls to be assertNotReadOnly(Value), and do an
additional check there, just in case somehow something is slipping
through.

The additional advantage to this approach is that we can then directly
call allowChange() before attempting to make a change, which could be
good from a user interaction standpoint.

-Patrick

On 8/29/07, Pinaki Poddar <ppoddar@bea.com> wrote:
>
> Some out-of-band discussion on proposed change of dynamic modification

> of Values.
>
>
> > -----Original Message-----
> > From: Pinaki Poddar
> > Sent: Wednesday, August 29, 2007 7:41 AM
> > To: Patrick Linskey
> > Subject: RE: EJB/Kodo team activity
> >
> > Hi,
> > I thought of prserving the oldValue and improve the computaion of 
> > hashCode based on our earlier discssion (I agree that a bug is 
> > lurking in purely supressing all dynamic values to compute 
> > Configuration.hashCode).
> >
> > But, currently all Value implementations update its internal state 
> > *before* notifying the listener on via valueChanged().
> > I did not know how to achieve that effect without changing all the 
> > Value implementaions.
> > Essentially internal state is overwritten -- I can (and I think we 
> > should) make the change across all Value implementations to first 
> > call valueChanged() (or rather
> > valueCahnaging()) so that copy-on-post-freeze-write can be in a 
> > single place at Value implementaion.
> > Also I propose to make Value.valueChanged() final if we are adopting

> > this current strategy in discussion.
> >
> > So to summarize following are the proposed changes:
> > 1. Rename ValueListener.valueChanged() to valueChanging() to reflect

> > the vetoing power of ValueListener 2. Reorder set() methods in all 
> > Value implementations to allow
> ValueListener
> > to veto and also to enable copy-on-post-freeze-write 3. Improve 
> > Configuration.hashCode computation for strong equality 4. Remove 
> > superfluous assertReadOnly 5. Make Value.valueChanged() final 6. 
> > reorder condition check for Value.isDynamic() to preempt a cast
> >
> >
> > Pinaki Poddar
> > 972.834.2865
> >
> > -----Original Message-----
> > From: Patrick Linskey
> > Sent: Wednesday, August 29, 2007 1:11 AM
> > To: Pinaki Poddar
> > Subject: RE: EJB/Kodo team activity
> >
> > Hi,
> >
> > It looks great.
> >
> > There is one more thing that I think that we can do pretty easily 
> > with this architecture: if a Value preserves what it was set to 
> > pre-freeze (probably through a copy-on-post-freeze-write algorithm),

> > then we can have a
> > Value.getOriginal() call that the hashCode() and equals() methods in

> > Configuration can use. By doing this, we can preserve strong 
> > equality whereby two configurations whose values were the same when 
> > they were frozen are equal, rather than disregarding dynamic 
> > properties altogether.
> >
> > Also, with the veto-change logic in place, I think that we can 
> > safely simplify the *ConfigurationImpl classes by removing all the 
> > assertNotReadOnly() calls, which should now be superfluous.
> >
> > Finally, a minor detail: if you move the 'this.isDynamic'
> > check to the beginning of that 'if' statement, the VM will be able 
> > to avoid a cast and a method call. Theoretically, I guess the JIT 
> > should take care of that, but it's always nice to line things up 
> > appropriately when possible.
> >
> > -Patrick
> >
> > --
> > Patrick Linskey
> > BEA Systems, Inc.
> > ______________________________________________________________
> >
> > > -----Original Message-----
> > > From: Pinaki Poddar
> > > Sent: Tuesday, August 28, 2007 7:54 PM
> > > To: Patrick Linskey
> > > Subject: RE: EJB/Kodo team activity
> > >
> > > Please comment if this is somewhat inline with your thinking...
> > > I have not removed the implementation of the APIs that I
> > added but I
> > > will...
> > >
> > > Index:
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Configur
> > > ation.java
> > > ==================================================================
> > > =
> > > ---
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Configur
> > > ation.java  (revision 568257)
> > > +++
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Configur
> > > ation.java  (working copy)
> > > @@ -230,21 +230,21 @@
> > >       *
> > >       * @since 1.0.0
> > >       */
> > > -    public void modifyDynamic(String property, Object newValue);
> > > -
> > > -    /**
> > > -     * Affirms if the given property can be modified
> > > <em>dynamically</em> i.e.
> > > -     * even after the receiver is {@link
> > > #setReadOnly(boolean) frozen}.
> > > -     *
> > > -     * @since 1.0.0
> > > -     */
> > > -    public boolean isDynamic(String property);
> > > -
> > > -    /**
> > > -     * Gets the values that can be modified
> > <em>dynamically</em> i.e.
> > > -     * even after the receiver is {@link
> > > #setReadOnly(boolean) frozen}.
> > > -     *
> > > -     * @since 1.0.0
> > > -     */
> > > -    public Value[] getDynamicValues();
> > > +//    public void modifyDynamic(String property, Object
newValue);
> > > +//
> > > +//    /**
> > > +//     * Affirms if the given property can be modified
> > > <em>dynamically</em> i.e.
> > > +//     * even after the receiver is {@link
> > > #setReadOnly(boolean) frozen}.
> > > +//     *
> > > +//     * @since 1.0.0
> > > +//     */
> > > +//    public boolean isDynamic(String property);
> > > +//
> > > +//    /**
> > > +//     * Gets the values that can be modified
> > > <em>dynamically</em> i.e.
> > > +//     * even after the receiver is {@link
> > > #setReadOnly(boolean) frozen}.
> > > +//     *
> > > +//     * @since 1.0.0
> > > +//     */
> > > +//    public Value[] getDynamicValues();
> > >  }
> > > Index:
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Value.java
> > > ==================================================================
> > > =
> > > ---
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Val
> > > ue.java     (revision 568257)
> > > +++
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Val
> > > ue.java     (working copy)
> > > @@ -43,6 +43,7 @@
> > >      private ValueListener listen = null;
> > >      private boolean aliasListComprehensive = false;
> > >      private Class scope = null;
> > > +    private boolean isDynamic = false;
> > >
> > >      /**
> > >       * Default constructor.
> > > @@ -345,9 +346,23 @@
> > >       * Subclasses should call this method when their inernal 
> > > value changes.
> > >       */
> > >      public void valueChanged() {
> > > -        if (listen != null)
> > > -            listen.valueChanged(this);
> > > +        if (listen != null) {
> > > +           if (listen instanceof Configuration &&
> > > +                   (((Configuration)listen).isReadOnly())
> > > && (!this.isDynamic)) {
> > > +                   throw new
> > > RuntimeException(s_loc.get("veto-change",
> > > +                           this.getString()).toString());
> > > +                   }
> > > +           listen.valueChanged(this);
> > > +        }
> > >      }
> > > +
> > > +    public void setDynamic(boolean flag) {
> > > +           isDynamic = flag;
> > > +    }
> > > +
> > > +    public boolean isDynamic() {
> > > +           return isDynamic;
> > > +    }
> > >
> > >      public int hashCode() {
> > >          String str = getString();
> > > Index:
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Configur
> > > ationImpl.java
> > > ==================================================================
> > > =
> > > ---
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Configur
> > > ationImpl.java      (revision 568257)
> > > +++
> > > openjpa-lib/src/main/java/org/apache/openjpa/lib/conf/Configur
> > > ationImpl.java      (working copy)
> > > @@ -1013,9 +1013,10 @@
> > >             if (map == null)
> > >                     return null;
> > >             Map copy = new HashMap(map);
> > > -           Value[] dynamicValues = getDynamicValues();
> > > -           for (int i=0; i<dynamicValues.length; i++) {
> > > -
> > > Configurations.removeProperty(dynamicValues[i].getProperty(), 
> > > copy);
> > > +           Value[] values = getValues();
> > > +           for (int i=0; i<values.length; i++) {
> > > +                   if (values[i].isDynamic())
> > > +
> > > Configurations.removeProperty(values[i].getProperty(), copy);
> > >             }
> > >             return copy;
> > >      }
> > > Index:
> > > openjpa-kernel/src/main/java/org/apache/openjpa/conf/OpenJPACo
> > > nfigurationImpl.java
> > > ==================================================================
> > > =
> > > ---
> > > openjpa-kernel/src/main/java/org/apache/openjpa/conf/OpenJPACo
> > > nfigurationImpl.java        (revision 568257)
> > > +++
> > > openjpa-kernel/src/main/java/org/apache/openjpa/conf/OpenJPACo
> > > nfigurationImpl.java        (working copy)
> > > @@ -209,6 +209,7 @@
> > >          dataCacheTimeout = addInt("DataCacheTimeout");
> > >          dataCacheTimeout.setDefault("-1");
> > >          dataCacheTimeout.set(-1);
> > > +        dataCacheTimeout.setDynamic(true);
> > >
> > >          queryCachePlugin = addPlugin("QueryCache", true);
> > >          aliases = new String[] {
> > > @@ -387,6 +388,7 @@
> > >          fetchBatchSize = addInt("FetchBatchSize");
> > >          fetchBatchSize.setDefault("-1");
> > >          fetchBatchSize.set(-1);
> > > +        fetchBatchSize.setDynamic(true);
> > >
> > >          maxFetchDepth = addInt("MaxFetchDepth");
> > >          maxFetchDepth.setDefault("-1"); @@ -410,7 +412,8 @@
> > >          lockTimeout = addInt("LockTimeout");
> > >          lockTimeout.setDefault("-1");
> > >          lockTimeout.set(-1);
> > > -
> > > +        lockTimeout.setDynamic(true);
> > > +
> > >          readLockLevel = addInt("ReadLockLevel");
> > >          aliases =
> > >              new String[] {
> > >
> > >
> > >
> > > Pinaki Poddar
> > > 972.834.2865
> > >
>
> 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.
>



--
Patrick Linskey
202 669 5907

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