openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Patrick Linskey" <plins...@gmail.com>
Subject Re: Discussion on proposed change of dynamic modification of Values
Date Wed, 29 Aug 2007 20:48:46 GMT
We should be able to easily enough change the internal boolean in
ConfigurationImpl to an int, and transition through a couple of states
during the freezing process.

Also, note that we'll also need to do checks in setString() and
setObject(), which are used by the non-ObjectValue Value subtypes.

-Patrick

On 8/29/07, Pinaki Poddar <ppoddar@bea.com> wrote:
>
> 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.
>


-- 
Patrick Linskey
202 669 5907

Mime
View raw message