You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openjpa.apache.org by Pinaki Poddar <pp...@bea.com> on 2007/08/29 15:24:53 UTC
Discussion on proposed change of dynamic modification of Values
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.
Re: Discussion on proposed change of dynamic modification of Values
Posted by Patrick Linskey <pl...@gmail.com>.
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 <pp...@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 <pp...@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
RE: Discussion on proposed change of dynamic modification of Values
Posted by Pinaki Poddar <pp...@bea.com>.
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 <pp...@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.
Re: Discussion on proposed change of dynamic modification of Values
Posted by Patrick Linskey <pl...@gmail.com>.
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 <pp...@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