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 <[EMAIL PROTECTED]> 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:[EMAIL PROTECTED] > Sent: Wednesday, August 29, 2007 11:06 AM > To: [email protected] > 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 <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] > > > > #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 [EMAIL PROTECTED] > > > > #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 [EMAIL PROTECTED] > > > > #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 [EMAIL PROTECTED] > > > > #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
