Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
24 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
[hidden email] wrote:

> Author: adrianc
> Date: Sun Aug  9 18:04:26 2009
> New Revision: 802567
>
> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
> Log:
> Refactored GenericDelegator:
>
> 1. Converted GenericDelegator to an interface. We already have DelegatorInterface, but it isn't being used anywhere. Removed DelegatorInterface.java.
>
> 2. Extracted the static, cached data from the GenericDelegator implementation and put it in its own class - DelegatorData. The GenericDelegator implementation holds a reference to the DelegatorData instance. This makes it possible to have per-thread instances of GenericDelegator.
>
> 3. Replaced the ThreadLocal variables with regular variables. ThreadLocal variables are no longer needed. Client code doesn't need to be concerned with pushing and popping the GenericDelegator state.
>
> This commit paves the way for the forthcoming ExecutionContext.
>
> User modifications will have to replace GenericDelegator.getGenericDelegator(...) with DelegatorFactory.getGenericDelegator(...). Also, replace the push code with the new setXxx methods, and remove the pop code. If modifications used DelegatorInterface, replace that with GenericDelegator.
>
> Aside from those changes, this commit is backwards compatible.

No, it is not backwards compatible.

When a java class is compiled, the bytecode requests an interface
named 'foo', or a class named 'foo'.  If 'foo' changes from class to
interface, then pre-compiled classes will *not* load.

Please, change GenericDelegator back to a class.

If DelegatorInterface wasn't used, and was just not uptodate with the
method signatures, wouldn't the simpler thing have been to improve
DelegatorInterface, then to change the class itself?  It seems more
work to change the class to an interface.

I have external code(webslinger) that needs to support multiple
versions of ofbiz(one all the way back to 512946).  This change makes
that impossible.  I have to have multiple versions of ofbiz
installed(pre/post this change), and compile the class once for each
ofbiz version.
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adrian Crum
Adam Heath wrote:

> [hidden email] wrote:
>> Author: adrianc
>> Date: Sun Aug  9 18:04:26 2009
>> New Revision: 802567
>>
>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>> Log:
>> Refactored GenericDelegator:
>>
>> 1. Converted GenericDelegator to an interface. We already have DelegatorInterface, but it isn't being used anywhere. Removed DelegatorInterface.java.
>>
>> 2. Extracted the static, cached data from the GenericDelegator implementation and put it in its own class - DelegatorData. The GenericDelegator implementation holds a reference to the DelegatorData instance. This makes it possible to have per-thread instances of GenericDelegator.
>>
>> 3. Replaced the ThreadLocal variables with regular variables. ThreadLocal variables are no longer needed. Client code doesn't need to be concerned with pushing and popping the GenericDelegator state.
>>
>> This commit paves the way for the forthcoming ExecutionContext.
>>
>> User modifications will have to replace GenericDelegator.getGenericDelegator(...) with DelegatorFactory.getGenericDelegator(...). Also, replace the push code with the new setXxx methods, and remove the pop code. If modifications used DelegatorInterface, replace that with GenericDelegator.
>>
>> Aside from those changes, this commit is backwards compatible.
>
> No, it is not backwards compatible.
>
> When a java class is compiled, the bytecode requests an interface
> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
> interface, then pre-compiled classes will *not* load.
>
> Please, change GenericDelegator back to a class.
>
> If DelegatorInterface wasn't used, and was just not uptodate with the
> method signatures, wouldn't the simpler thing have been to improve
> DelegatorInterface, then to change the class itself?  It seems more
> work to change the class to an interface.
>
> I have external code(webslinger) that needs to support multiple
> versions of ofbiz(one all the way back to 512946).  This change makes
> that impossible.  I have to have multiple versions of ofbiz
> installed(pre/post this change), and compile the class once for each
> ofbiz version.

Which is easier: rewrite all your Webslinger code to reference
DelegatorInterface instead of GenericDelegator, or just recompile your
existing code without making any changes?

-Adrian

Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
Adrian Crum wrote:

> Adam Heath wrote:
>> [hidden email] wrote:
>>> Author: adrianc
>>> Date: Sun Aug  9 18:04:26 2009
>>> New Revision: 802567
>>>
>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>> Log:
>>> Refactored GenericDelegator:
>>>
>>> 1. Converted GenericDelegator to an interface. We already have
>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>> DelegatorInterface.java.
>>>
>>> 2. Extracted the static, cached data from the GenericDelegator
>>> implementation and put it in its own class - DelegatorData. The
>>> GenericDelegator implementation holds a reference to the
>>> DelegatorData instance. This makes it possible to have per-thread
>>> instances of GenericDelegator.
>>>
>>> 3. Replaced the ThreadLocal variables with regular variables.
>>> ThreadLocal variables are no longer needed. Client code doesn't need
>>> to be concerned with pushing and popping the GenericDelegator state.
>>>
>>> This commit paves the way for the forthcoming ExecutionContext.
>>>
>>> User modifications will have to replace
>>> GenericDelegator.getGenericDelegator(...) with
>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>> code with the new setXxx methods, and remove the pop code. If
>>> modifications used DelegatorInterface, replace that with
>>> GenericDelegator.
>>>
>>> Aside from those changes, this commit is backwards compatible.
>>
>> No, it is not backwards compatible.
>>
>> When a java class is compiled, the bytecode requests an interface
>> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
>> interface, then pre-compiled classes will *not* load.
>>
>> Please, change GenericDelegator back to a class.
>>
>> If DelegatorInterface wasn't used, and was just not uptodate with the
>> method signatures, wouldn't the simpler thing have been to improve
>> DelegatorInterface, then to change the class itself?  It seems more
>> work to change the class to an interface.
>>
>> I have external code(webslinger) that needs to support multiple
>> versions of ofbiz(one all the way back to 512946).  This change makes
>> that impossible.  I have to have multiple versions of ofbiz
>> installed(pre/post this change), and compile the class once for each
>> ofbiz version.
>
> Which is easier: rewrite all your Webslinger code to reference
> DelegatorInterface instead of GenericDelegator, or just recompile your
> existing code without making any changes?

That's just it, I wouldn't have to recompile *at all*, if
GenericDelegator stayed a class.  Nor would anyone else.

Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> [hidden email] wrote:
>>>> Author: adrianc
>>>> Date: Sun Aug  9 18:04:26 2009
>>>> New Revision: 802567
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>> Log:
>>>> Refactored GenericDelegator:
>>>>
>>>> 1. Converted GenericDelegator to an interface. We already have
>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>> DelegatorInterface.java.
>>>>
>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>> implementation and put it in its own class - DelegatorData. The
>>>> GenericDelegator implementation holds a reference to the
>>>> DelegatorData instance. This makes it possible to have per-thread
>>>> instances of GenericDelegator.
>>>>
>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>> ThreadLocal variables are no longer needed. Client code doesn't need
>>>> to be concerned with pushing and popping the GenericDelegator state.
>>>>
>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>
>>>> User modifications will have to replace
>>>> GenericDelegator.getGenericDelegator(...) with
>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>>> code with the new setXxx methods, and remove the pop code. If
>>>> modifications used DelegatorInterface, replace that with
>>>> GenericDelegator.
>>>>
>>>> Aside from those changes, this commit is backwards compatible.
>>> No, it is not backwards compatible.
>>>
>>> When a java class is compiled, the bytecode requests an interface
>>> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
>>> interface, then pre-compiled classes will *not* load.
>>>
>>> Please, change GenericDelegator back to a class.
>>>
>>> If DelegatorInterface wasn't used, and was just not uptodate with the
>>> method signatures, wouldn't the simpler thing have been to improve
>>> DelegatorInterface, then to change the class itself?  It seems more
>>> work to change the class to an interface.
>>>
>>> I have external code(webslinger) that needs to support multiple
>>> versions of ofbiz(one all the way back to 512946).  This change makes
>>> that impossible.  I have to have multiple versions of ofbiz
>>> installed(pre/post this change), and compile the class once for each
>>> ofbiz version.
>> Which is easier: rewrite all your Webslinger code to reference
>> DelegatorInterface instead of GenericDelegator, or just recompile your
>> existing code without making any changes?
>
> That's just it, I wouldn't have to recompile *at all*, if
> GenericDelegator stayed a class.  Nor would anyone else.
>

Namely, it's a conflict with invokevirtual vs. invokeinterface.

   4:   invokevirtual   #45; //Method
org/ofbiz/entity/GenericDelegator.getCache:()Lorg/ofbiz/entity/cache/Cache;


Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adrian Crum
In reply to this post by Adam Heath-2
Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> [hidden email] wrote:
>>>> Author: adrianc
>>>> Date: Sun Aug  9 18:04:26 2009
>>>> New Revision: 802567
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>> Log:
>>>> Refactored GenericDelegator:
>>>>
>>>> 1. Converted GenericDelegator to an interface. We already have
>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>> DelegatorInterface.java.
>>>>
>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>> implementation and put it in its own class - DelegatorData. The
>>>> GenericDelegator implementation holds a reference to the
>>>> DelegatorData instance. This makes it possible to have per-thread
>>>> instances of GenericDelegator.
>>>>
>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>> ThreadLocal variables are no longer needed. Client code doesn't need
>>>> to be concerned with pushing and popping the GenericDelegator state.
>>>>
>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>
>>>> User modifications will have to replace
>>>> GenericDelegator.getGenericDelegator(...) with
>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>>> code with the new setXxx methods, and remove the pop code. If
>>>> modifications used DelegatorInterface, replace that with
>>>> GenericDelegator.
>>>>
>>>> Aside from those changes, this commit is backwards compatible.
>>> No, it is not backwards compatible.
>>>
>>> When a java class is compiled, the bytecode requests an interface
>>> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
>>> interface, then pre-compiled classes will *not* load.
>>>
>>> Please, change GenericDelegator back to a class.
>>>
>>> If DelegatorInterface wasn't used, and was just not uptodate with the
>>> method signatures, wouldn't the simpler thing have been to improve
>>> DelegatorInterface, then to change the class itself?  It seems more
>>> work to change the class to an interface.
>>>
>>> I have external code(webslinger) that needs to support multiple
>>> versions of ofbiz(one all the way back to 512946).  This change makes
>>> that impossible.  I have to have multiple versions of ofbiz
>>> installed(pre/post this change), and compile the class once for each
>>> ofbiz version.
>> Which is easier: rewrite all your Webslinger code to reference
>> DelegatorInterface instead of GenericDelegator, or just recompile your
>> existing code without making any changes?
>
> That's just it, I wouldn't have to recompile *at all*, if
> GenericDelegator stayed a class.  Nor would anyone else.

I don't have a problem with reverting it, but GenericDelegator will
become an interface eventually. If you take a look at David's
ExecutionContext branch, that is what he has planned.

-Adrian
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
Adrian Crum wrote:

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> [hidden email] wrote:
>>>>> Author: adrianc
>>>>> Date: Sun Aug  9 18:04:26 2009
>>>>> New Revision: 802567
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>>> Log:
>>>>> Refactored GenericDelegator:
>>>>>
>>>>> 1. Converted GenericDelegator to an interface. We already have
>>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>>> DelegatorInterface.java.
>>>>>
>>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>>> implementation and put it in its own class - DelegatorData. The
>>>>> GenericDelegator implementation holds a reference to the
>>>>> DelegatorData instance. This makes it possible to have per-thread
>>>>> instances of GenericDelegator.
>>>>>
>>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>>> ThreadLocal variables are no longer needed. Client code doesn't need
>>>>> to be concerned with pushing and popping the GenericDelegator state.
>>>>>
>>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>>
>>>>> User modifications will have to replace
>>>>> GenericDelegator.getGenericDelegator(...) with
>>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>>>> code with the new setXxx methods, and remove the pop code. If
>>>>> modifications used DelegatorInterface, replace that with
>>>>> GenericDelegator.
>>>>>
>>>>> Aside from those changes, this commit is backwards compatible.
>>>> No, it is not backwards compatible.
>>>>
>>>> When a java class is compiled, the bytecode requests an interface
>>>> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
>>>> interface, then pre-compiled classes will *not* load.
>>>>
>>>> Please, change GenericDelegator back to a class.
>>>>
>>>> If DelegatorInterface wasn't used, and was just not uptodate with the
>>>> method signatures, wouldn't the simpler thing have been to improve
>>>> DelegatorInterface, then to change the class itself?  It seems more
>>>> work to change the class to an interface.
>>>>
>>>> I have external code(webslinger) that needs to support multiple
>>>> versions of ofbiz(one all the way back to 512946).  This change makes
>>>> that impossible.  I have to have multiple versions of ofbiz
>>>> installed(pre/post this change), and compile the class once for each
>>>> ofbiz version.
>>> Which is easier: rewrite all your Webslinger code to reference
>>> DelegatorInterface instead of GenericDelegator, or just recompile your
>>> existing code without making any changes?
>>
>> That's just it, I wouldn't have to recompile *at all*, if
>> GenericDelegator stayed a class.  Nor would anyone else.
>
> I don't have a problem with reverting it, but GenericDelegator will
> become an interface eventually. If you take a look at David's
> ExecutionContext branch, that is what he has planned.

Why?  We already had DelegatorInterface, that has existed for years;
it was just never fleshed out.

Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adrian Crum
Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> Adam Heath wrote:
>>>>> [hidden email] wrote:
>>>>>> Author: adrianc
>>>>>> Date: Sun Aug  9 18:04:26 2009
>>>>>> New Revision: 802567
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>>>> Log:
>>>>>> Refactored GenericDelegator:
>>>>>>
>>>>>> 1. Converted GenericDelegator to an interface. We already have
>>>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>>>> DelegatorInterface.java.
>>>>>>
>>>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>>>> implementation and put it in its own class - DelegatorData. The
>>>>>> GenericDelegator implementation holds a reference to the
>>>>>> DelegatorData instance. This makes it possible to have per-thread
>>>>>> instances of GenericDelegator.
>>>>>>
>>>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>>>> ThreadLocal variables are no longer needed. Client code doesn't need
>>>>>> to be concerned with pushing and popping the GenericDelegator state.
>>>>>>
>>>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>>>
>>>>>> User modifications will have to replace
>>>>>> GenericDelegator.getGenericDelegator(...) with
>>>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>>>>> code with the new setXxx methods, and remove the pop code. If
>>>>>> modifications used DelegatorInterface, replace that with
>>>>>> GenericDelegator.
>>>>>>
>>>>>> Aside from those changes, this commit is backwards compatible.
>>>>> No, it is not backwards compatible.
>>>>>
>>>>> When a java class is compiled, the bytecode requests an interface
>>>>> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
>>>>> interface, then pre-compiled classes will *not* load.
>>>>>
>>>>> Please, change GenericDelegator back to a class.
>>>>>
>>>>> If DelegatorInterface wasn't used, and was just not uptodate with the
>>>>> method signatures, wouldn't the simpler thing have been to improve
>>>>> DelegatorInterface, then to change the class itself?  It seems more
>>>>> work to change the class to an interface.
>>>>>
>>>>> I have external code(webslinger) that needs to support multiple
>>>>> versions of ofbiz(one all the way back to 512946).  This change makes
>>>>> that impossible.  I have to have multiple versions of ofbiz
>>>>> installed(pre/post this change), and compile the class once for each
>>>>> ofbiz version.
>>>> Which is easier: rewrite all your Webslinger code to reference
>>>> DelegatorInterface instead of GenericDelegator, or just recompile your
>>>> existing code without making any changes?
>>> That's just it, I wouldn't have to recompile *at all*, if
>>> GenericDelegator stayed a class.  Nor would anyone else.
>> I don't have a problem with reverting it, but GenericDelegator will
>> become an interface eventually. If you take a look at David's
>> ExecutionContext branch, that is what he has planned.
>
> Why?  We already had DelegatorInterface, that has existed for years;
> it was just never fleshed out.

It's only one class in a number of framework classes that will be
converted to interfaces. There has been considerable discussion about it
for about three months now.


-Adrian
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

David E. Jones-2
In reply to this post by Adam Heath-2

On Sep 8, 2009, at 11:58 AM, Adam Heath wrote:

> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> Adam Heath wrote:
>>>>> [hidden email] wrote:
>>>>>> Author: adrianc
>>>>>> Date: Sun Aug  9 18:04:26 2009
>>>>>> New Revision: 802567
>>>>>>
>>>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>>>> Log:
>>>>>> Refactored GenericDelegator:
>>>>>>
>>>>>> 1. Converted GenericDelegator to an interface. We already have
>>>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>>>> DelegatorInterface.java.
>>>>>>
>>>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>>>> implementation and put it in its own class - DelegatorData. The
>>>>>> GenericDelegator implementation holds a reference to the
>>>>>> DelegatorData instance. This makes it possible to have per-thread
>>>>>> instances of GenericDelegator.
>>>>>>
>>>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>>>> ThreadLocal variables are no longer needed. Client code doesn't  
>>>>>> need
>>>>>> to be concerned with pushing and popping the GenericDelegator  
>>>>>> state.
>>>>>>
>>>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>>>
>>>>>> User modifications will have to replace
>>>>>> GenericDelegator.getGenericDelegator(...) with
>>>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>>>>> code with the new setXxx methods, and remove the pop code. If
>>>>>> modifications used DelegatorInterface, replace that with
>>>>>> GenericDelegator.
>>>>>>
>>>>>> Aside from those changes, this commit is backwards compatible.
>>>>> No, it is not backwards compatible.
>>>>>
>>>>> When a java class is compiled, the bytecode requests an interface
>>>>> named 'foo', or a class named 'foo'.  If 'foo' changes from  
>>>>> class to
>>>>> interface, then pre-compiled classes will *not* load.
>>>>>
>>>>> Please, change GenericDelegator back to a class.
>>>>>
>>>>> If DelegatorInterface wasn't used, and was just not uptodate  
>>>>> with the
>>>>> method signatures, wouldn't the simpler thing have been to improve
>>>>> DelegatorInterface, then to change the class itself?  It seems  
>>>>> more
>>>>> work to change the class to an interface.
>>>>>
>>>>> I have external code(webslinger) that needs to support multiple
>>>>> versions of ofbiz(one all the way back to 512946).  This change  
>>>>> makes
>>>>> that impossible.  I have to have multiple versions of ofbiz
>>>>> installed(pre/post this change), and compile the class once for  
>>>>> each
>>>>> ofbiz version.
>>>> Which is easier: rewrite all your Webslinger code to reference
>>>> DelegatorInterface instead of GenericDelegator, or just recompile  
>>>> your
>>>> existing code without making any changes?
>>>
>>> That's just it, I wouldn't have to recompile *at all*, if
>>> GenericDelegator stayed a class.  Nor would anyone else.
>>
>> I don't have a problem with reverting it, but GenericDelegator will
>> become an interface eventually. If you take a look at David's
>> ExecutionContext branch, that is what he has planned.
>
> Why?  We already had DelegatorInterface, that has existed for years;
> it was just never fleshed out.

There was a lot of discussion about this in the context of the reasons/
goals behind the executioncontext branch(es). The basic idea is to  
move the framework to be accessible through a set of interfaces that  
are in a single place, a low-level component that other framework  
components will depend on, and then all code will use the framework  
through the interfaces instead of going directly to classes (which  
will just be internal implementations of the framework interfaces and  
not generally used directly).

For the rationale behind this, please see the write up I sent to this  
mailing list a few weeks ago about the ExecutionContext and related  
stuff.

-David


Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
David E Jones wrote:

>
> On Sep 8, 2009, at 11:58 AM, Adam Heath wrote:
>
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> Adrian Crum wrote:
>>>>> Adam Heath wrote:
>>>>>> [hidden email] wrote:
>>>>>>> Author: adrianc
>>>>>>> Date: Sun Aug  9 18:04:26 2009
>>>>>>> New Revision: 802567
>>>>>>>
>>>>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>>>>> Log:
>>>>>>> Refactored GenericDelegator:
>>>>>>>
>>>>>>> 1. Converted GenericDelegator to an interface. We already have
>>>>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>>>>> DelegatorInterface.java.
>>>>>>>
>>>>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>>>>> implementation and put it in its own class - DelegatorData. The
>>>>>>> GenericDelegator implementation holds a reference to the
>>>>>>> DelegatorData instance. This makes it possible to have per-thread
>>>>>>> instances of GenericDelegator.
>>>>>>>
>>>>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>>>>> ThreadLocal variables are no longer needed. Client code doesn't need
>>>>>>> to be concerned with pushing and popping the GenericDelegator state.
>>>>>>>
>>>>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>>>>
>>>>>>> User modifications will have to replace
>>>>>>> GenericDelegator.getGenericDelegator(...) with
>>>>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>>>>>> code with the new setXxx methods, and remove the pop code. If
>>>>>>> modifications used DelegatorInterface, replace that with
>>>>>>> GenericDelegator.
>>>>>>>
>>>>>>> Aside from those changes, this commit is backwards compatible.
>>>>>> No, it is not backwards compatible.
>>>>>>
>>>>>> When a java class is compiled, the bytecode requests an interface
>>>>>> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
>>>>>> interface, then pre-compiled classes will *not* load.
>>>>>>
>>>>>> Please, change GenericDelegator back to a class.
>>>>>>
>>>>>> If DelegatorInterface wasn't used, and was just not uptodate with the
>>>>>> method signatures, wouldn't the simpler thing have been to improve
>>>>>> DelegatorInterface, then to change the class itself?  It seems more
>>>>>> work to change the class to an interface.
>>>>>>
>>>>>> I have external code(webslinger) that needs to support multiple
>>>>>> versions of ofbiz(one all the way back to 512946).  This change makes
>>>>>> that impossible.  I have to have multiple versions of ofbiz
>>>>>> installed(pre/post this change), and compile the class once for each
>>>>>> ofbiz version.
>>>>> Which is easier: rewrite all your Webslinger code to reference
>>>>> DelegatorInterface instead of GenericDelegator, or just recompile your
>>>>> existing code without making any changes?
>>>>
>>>> That's just it, I wouldn't have to recompile *at all*, if
>>>> GenericDelegator stayed a class.  Nor would anyone else.
>>>
>>> I don't have a problem with reverting it, but GenericDelegator will
>>> become an interface eventually. If you take a look at David's
>>> ExecutionContext branch, that is what he has planned.
>>
>> Why?  We already had DelegatorInterface, that has existed for years;
>> it was just never fleshed out.
>
> There was a lot of discussion about this in the context of the
> reasons/goals behind the executioncontext branch(es). The basic idea is
> to move the framework to be accessible through a set of interfaces that
> are in a single place, a low-level component that other framework
> components will depend on, and then all code will use the framework
> through the interfaces instead of going directly to classes (which will
> just be internal implementations of the framework interfaces and not
> generally used directly).
>
> For the rationale behind this, please see the write up I sent to this
> mailing list a few weeks ago about the ExecutionContext and related stuff.

Sure, understand all that.  But this broke ABI.  And I understand that
such a set of changes will *have* to break ABI.

If this change is to support the ExecutionContext branch, then it
should stay in that branch, 'til it is ready.

I'm seeing this done in the reverse order.  Introduce a *new*
interface, that existing implementations will use.  Alter all users to
use the interface.  When all users are so modified, rename the
implementation to whatever.

What has happened, is that the last step was done first.
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

David E. Jones-2

On Sep 8, 2009, at 1:18 PM, Adam Heath wrote:

> David E Jones wrote:
>>
>> On Sep 8, 2009, at 11:58 AM, Adam Heath wrote:
>>
>>> Adrian Crum wrote:
>>>> Adam Heath wrote:
>>>>> Adrian Crum wrote:
>>>>>> Adam Heath wrote:
>>>>>>> [hidden email] wrote:
>>>>>>>> Author: adrianc
>>>>>>>> Date: Sun Aug  9 18:04:26 2009
>>>>>>>> New Revision: 802567
>>>>>>>>
>>>>>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>>>>>> Log:
>>>>>>>> Refactored GenericDelegator:
>>>>>>>>
>>>>>>>> 1. Converted GenericDelegator to an interface. We already have
>>>>>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>>>>>> DelegatorInterface.java.
>>>>>>>>
>>>>>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>>>>>> implementation and put it in its own class - DelegatorData. The
>>>>>>>> GenericDelegator implementation holds a reference to the
>>>>>>>> DelegatorData instance. This makes it possible to have per-
>>>>>>>> thread
>>>>>>>> instances of GenericDelegator.
>>>>>>>>
>>>>>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>>>>>> ThreadLocal variables are no longer needed. Client code  
>>>>>>>> doesn't need
>>>>>>>> to be concerned with pushing and popping the GenericDelegator  
>>>>>>>> state.
>>>>>>>>
>>>>>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>>>>>
>>>>>>>> User modifications will have to replace
>>>>>>>> GenericDelegator.getGenericDelegator(...) with
>>>>>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the  
>>>>>>>> push
>>>>>>>> code with the new setXxx methods, and remove the pop code. If
>>>>>>>> modifications used DelegatorInterface, replace that with
>>>>>>>> GenericDelegator.
>>>>>>>>
>>>>>>>> Aside from those changes, this commit is backwards compatible.
>>>>>>> No, it is not backwards compatible.
>>>>>>>
>>>>>>> When a java class is compiled, the bytecode requests an  
>>>>>>> interface
>>>>>>> named 'foo', or a class named 'foo'.  If 'foo' changes from  
>>>>>>> class to
>>>>>>> interface, then pre-compiled classes will *not* load.
>>>>>>>
>>>>>>> Please, change GenericDelegator back to a class.
>>>>>>>
>>>>>>> If DelegatorInterface wasn't used, and was just not uptodate  
>>>>>>> with the
>>>>>>> method signatures, wouldn't the simpler thing have been to  
>>>>>>> improve
>>>>>>> DelegatorInterface, then to change the class itself?  It seems  
>>>>>>> more
>>>>>>> work to change the class to an interface.
>>>>>>>
>>>>>>> I have external code(webslinger) that needs to support multiple
>>>>>>> versions of ofbiz(one all the way back to 512946).  This  
>>>>>>> change makes
>>>>>>> that impossible.  I have to have multiple versions of ofbiz
>>>>>>> installed(pre/post this change), and compile the class once  
>>>>>>> for each
>>>>>>> ofbiz version.
>>>>>> Which is easier: rewrite all your Webslinger code to reference
>>>>>> DelegatorInterface instead of GenericDelegator, or just  
>>>>>> recompile your
>>>>>> existing code without making any changes?
>>>>>
>>>>> That's just it, I wouldn't have to recompile *at all*, if
>>>>> GenericDelegator stayed a class.  Nor would anyone else.
>>>>
>>>> I don't have a problem with reverting it, but GenericDelegator will
>>>> become an interface eventually. If you take a look at David's
>>>> ExecutionContext branch, that is what he has planned.
>>>
>>> Why?  We already had DelegatorInterface, that has existed for years;
>>> it was just never fleshed out.
>>
>> There was a lot of discussion about this in the context of the
>> reasons/goals behind the executioncontext branch(es). The basic  
>> idea is
>> to move the framework to be accessible through a set of interfaces  
>> that
>> are in a single place, a low-level component that other framework
>> components will depend on, and then all code will use the framework
>> through the interfaces instead of going directly to classes (which  
>> will
>> just be internal implementations of the framework interfaces and not
>> generally used directly).
>>
>> For the rationale behind this, please see the write up I sent to this
>> mailing list a few weeks ago about the ExecutionContext and related  
>> stuff.
>
> Sure, understand all that.  But this broke ABI.  And I understand that
> such a set of changes will *have* to break ABI.
>
> If this change is to support the ExecutionContext branch, then it
> should stay in that branch, 'til it is ready.

That is one thing I DO agree with, which is why I was doing all of the  
work I did in a branch and refactoring dependent code there.

> I'm seeing this done in the reverse order.  Introduce a *new*
> interface, that existing implementations will use.  Alter all users to
> use the interface.  When all users are so modified, rename the
> implementation to whatever.
>
> What has happened, is that the last step was done first.

Profound.

-David


Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adrian Crum
In reply to this post by Adam Heath-2
Adam Heath wrote:

> David E Jones wrote:
>> On Sep 8, 2009, at 11:58 AM, Adam Heath wrote:
>>
>>> Adrian Crum wrote:
>>>> Adam Heath wrote:
>>>>> Adrian Crum wrote:
>>>>>> Adam Heath wrote:
>>>>>>> [hidden email] wrote:
>>>>>>>> Author: adrianc
>>>>>>>> Date: Sun Aug  9 18:04:26 2009
>>>>>>>> New Revision: 802567
>>>>>>>>
>>>>>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>>>>>> Log:
>>>>>>>> Refactored GenericDelegator:
>>>>>>>>
>>>>>>>> 1. Converted GenericDelegator to an interface. We already have
>>>>>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>>>>>> DelegatorInterface.java.
>>>>>>>>
>>>>>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>>>>>> implementation and put it in its own class - DelegatorData. The
>>>>>>>> GenericDelegator implementation holds a reference to the
>>>>>>>> DelegatorData instance. This makes it possible to have per-thread
>>>>>>>> instances of GenericDelegator.
>>>>>>>>
>>>>>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>>>>>> ThreadLocal variables are no longer needed. Client code doesn't need
>>>>>>>> to be concerned with pushing and popping the GenericDelegator state.
>>>>>>>>
>>>>>>>> This commit paves the way for the forthcoming ExecutionContext.
>>>>>>>>
>>>>>>>> User modifications will have to replace
>>>>>>>> GenericDelegator.getGenericDelegator(...) with
>>>>>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the push
>>>>>>>> code with the new setXxx methods, and remove the pop code. If
>>>>>>>> modifications used DelegatorInterface, replace that with
>>>>>>>> GenericDelegator.
>>>>>>>>
>>>>>>>> Aside from those changes, this commit is backwards compatible.
>>>>>>> No, it is not backwards compatible.
>>>>>>>
>>>>>>> When a java class is compiled, the bytecode requests an interface
>>>>>>> named 'foo', or a class named 'foo'.  If 'foo' changes from class to
>>>>>>> interface, then pre-compiled classes will *not* load.
>>>>>>>
>>>>>>> Please, change GenericDelegator back to a class.
>>>>>>>
>>>>>>> If DelegatorInterface wasn't used, and was just not uptodate with the
>>>>>>> method signatures, wouldn't the simpler thing have been to improve
>>>>>>> DelegatorInterface, then to change the class itself?  It seems more
>>>>>>> work to change the class to an interface.
>>>>>>>
>>>>>>> I have external code(webslinger) that needs to support multiple
>>>>>>> versions of ofbiz(one all the way back to 512946).  This change makes
>>>>>>> that impossible.  I have to have multiple versions of ofbiz
>>>>>>> installed(pre/post this change), and compile the class once for each
>>>>>>> ofbiz version.
>>>>>> Which is easier: rewrite all your Webslinger code to reference
>>>>>> DelegatorInterface instead of GenericDelegator, or just recompile your
>>>>>> existing code without making any changes?
>>>>> That's just it, I wouldn't have to recompile *at all*, if
>>>>> GenericDelegator stayed a class.  Nor would anyone else.
>>>> I don't have a problem with reverting it, but GenericDelegator will
>>>> become an interface eventually. If you take a look at David's
>>>> ExecutionContext branch, that is what he has planned.
>>> Why?  We already had DelegatorInterface, that has existed for years;
>>> it was just never fleshed out.
>> There was a lot of discussion about this in the context of the
>> reasons/goals behind the executioncontext branch(es). The basic idea is
>> to move the framework to be accessible through a set of interfaces that
>> are in a single place, a low-level component that other framework
>> components will depend on, and then all code will use the framework
>> through the interfaces instead of going directly to classes (which will
>> just be internal implementations of the framework interfaces and not
>> generally used directly).
>>
>> For the rationale behind this, please see the write up I sent to this
>> mailing list a few weeks ago about the ExecutionContext and related stuff.
>
> Sure, understand all that.  But this broke ABI.  And I understand that
> such a set of changes will *have* to break ABI.
>
> If this change is to support the ExecutionContext branch, then it
> should stay in that branch, 'til it is ready.

David and I are in agreement on the end result, and this point is where
we disagree. Like you, David wants all changes to be made in the branch.
I don't think a branch is needed. The interface extractions can be done
a little at a time in the trunk.

A branch with the kind of extensive changes that are planned will open
up a HUGE can of worms when it is merged with the trunk. Consider that
this one interface extraction resulted in three bug reports - what would
happen if we introduced dozens of interface extractions in a single commit?

Anyways, I don't mind reverting it. I'm just making the point that you
will have to resolve these issues at some time in the future.

-Adrian

Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Tim Ruppert

On Sep 8, 2009, at 1:36 PM, Adrian Crum wrote:

> Adam Heath wrote:
>> David E Jones wrote:
>>> On Sep 8, 2009, at 11:58 AM, Adam Heath wrote:
>>>
>>>> Adrian Crum wrote:
>>>>> Adam Heath wrote:
>>>>>> Adrian Crum wrote:
>>>>>>> Adam Heath wrote:
>>>>>>>> [hidden email] wrote:
>>>>>>>>> Author: adrianc
>>>>>>>>> Date: Sun Aug  9 18:04:26 2009
>>>>>>>>> New Revision: 802567
>>>>>>>>>
>>>>>>>>> URL: http://svn.apache.org/viewvc?rev=802567&view=rev
>>>>>>>>> Log:
>>>>>>>>> Refactored GenericDelegator:
>>>>>>>>>
>>>>>>>>> 1. Converted GenericDelegator to an interface. We already have
>>>>>>>>> DelegatorInterface, but it isn't being used anywhere. Removed
>>>>>>>>> DelegatorInterface.java.
>>>>>>>>>
>>>>>>>>> 2. Extracted the static, cached data from the GenericDelegator
>>>>>>>>> implementation and put it in its own class - DelegatorData.  
>>>>>>>>> The
>>>>>>>>> GenericDelegator implementation holds a reference to the
>>>>>>>>> DelegatorData instance. This makes it possible to have per-
>>>>>>>>> thread
>>>>>>>>> instances of GenericDelegator.
>>>>>>>>>
>>>>>>>>> 3. Replaced the ThreadLocal variables with regular variables.
>>>>>>>>> ThreadLocal variables are no longer needed. Client code  
>>>>>>>>> doesn't need
>>>>>>>>> to be concerned with pushing and popping the  
>>>>>>>>> GenericDelegator state.
>>>>>>>>>
>>>>>>>>> This commit paves the way for the forthcoming  
>>>>>>>>> ExecutionContext.
>>>>>>>>>
>>>>>>>>> User modifications will have to replace
>>>>>>>>> GenericDelegator.getGenericDelegator(...) with
>>>>>>>>> DelegatorFactory.getGenericDelegator(...). Also, replace the  
>>>>>>>>> push
>>>>>>>>> code with the new setXxx methods, and remove the pop code. If
>>>>>>>>> modifications used DelegatorInterface, replace that with
>>>>>>>>> GenericDelegator.
>>>>>>>>>
>>>>>>>>> Aside from those changes, this commit is backwards compatible.
>>>>>>>> No, it is not backwards compatible.
>>>>>>>>
>>>>>>>> When a java class is compiled, the bytecode requests an  
>>>>>>>> interface
>>>>>>>> named 'foo', or a class named 'foo'.  If 'foo' changes from  
>>>>>>>> class to
>>>>>>>> interface, then pre-compiled classes will *not* load.
>>>>>>>>
>>>>>>>> Please, change GenericDelegator back to a class.
>>>>>>>>
>>>>>>>> If DelegatorInterface wasn't used, and was just not uptodate  
>>>>>>>> with the
>>>>>>>> method signatures, wouldn't the simpler thing have been to  
>>>>>>>> improve
>>>>>>>> DelegatorInterface, then to change the class itself?  It  
>>>>>>>> seems more
>>>>>>>> work to change the class to an interface.
>>>>>>>>
>>>>>>>> I have external code(webslinger) that needs to support multiple
>>>>>>>> versions of ofbiz(one all the way back to 512946).  This  
>>>>>>>> change makes
>>>>>>>> that impossible.  I have to have multiple versions of ofbiz
>>>>>>>> installed(pre/post this change), and compile the class once  
>>>>>>>> for each
>>>>>>>> ofbiz version.
>>>>>>> Which is easier: rewrite all your Webslinger code to reference
>>>>>>> DelegatorInterface instead of GenericDelegator, or just  
>>>>>>> recompile your
>>>>>>> existing code without making any changes?
>>>>>> That's just it, I wouldn't have to recompile *at all*, if
>>>>>> GenericDelegator stayed a class.  Nor would anyone else.
>>>>> I don't have a problem with reverting it, but GenericDelegator  
>>>>> will
>>>>> become an interface eventually. If you take a look at David's
>>>>> ExecutionContext branch, that is what he has planned.
>>>> Why?  We already had DelegatorInterface, that has existed for  
>>>> years;
>>>> it was just never fleshed out.
>>> There was a lot of discussion about this in the context of the
>>> reasons/goals behind the executioncontext branch(es). The basic  
>>> idea is
>>> to move the framework to be accessible through a set of interfaces  
>>> that
>>> are in a single place, a low-level component that other framework
>>> components will depend on, and then all code will use the framework
>>> through the interfaces instead of going directly to classes (which  
>>> will
>>> just be internal implementations of the framework interfaces and not
>>> generally used directly).
>>>
>>> For the rationale behind this, please see the write up I sent to  
>>> this
>>> mailing list a few weeks ago about the ExecutionContext and  
>>> related stuff.
>> Sure, understand all that.  But this broke ABI.  And I understand  
>> that
>> such a set of changes will *have* to break ABI.
>> If this change is to support the ExecutionContext branch, then it
>> should stay in that branch, 'til it is ready.
>
> David and I are in agreement on the end result, and this point is  
> where we disagree. Like you, David wants all changes to be made in  
> the branch. I don't think a branch is needed. The interface  
> extractions can be done a little at a time in the trunk.
>
> A branch with the kind of extensive changes that are planned will  
> open up a HUGE can of worms when it is merged with the trunk.  
> Consider that this one interface extraction resulted in three bug  
> reports - what would happen if we introduced dozens of interface  
> extractions in a single commit?
>
> Anyways, I don't mind reverting it. I'm just making the point that  
> you will have to resolve these issues at some time in the future.
I have to admit that keeping everything outside and then merging  
things back is HUGE risk without proper unit tests on everything that  
it will touch.  In this case - that would be everything in the  
system.  I would rather it be incremental once the plan is laid down  
rather than hitting all at once.

>
> -Adrian
>


smime.p7s (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
In reply to this post by Adrian Crum
Adrian Crum wrote:
> David and I are in agreement on the end result, and this point is where
> we disagree. Like you, David wants all changes to be made in the branch.
> I don't think a branch is needed. The interface extractions can be done
> a little at a time in the trunk.
>
> A branch with the kind of extensive changes that are planned will open
> up a HUGE can of worms when it is merged with the trunk. Consider that
> this one interface extraction resulted in three bug reports - what would
> happen if we introduced dozens of interface extractions in a single commit?

I agree with you as well, this does *not* need to be done in a
separate branch.

However, what you did by doing the last step first, in trunk, is the
wrong approach.

* Adding a new interface to some base component does not break
unreleated code(short amount of time).
* Implementing said interface by an existing object does not break
unreleated code(short amount of time).
* Modifying other code to use the interface does not break unrelated
code(takes a long time to do, and can be done incrementally by many
people).
* When other code no longer references concrete class, then
remove/rename it(breaks unreleated code, but that needs to be
announced, not just discussed).

This set of changes does *not* need to be done in a hole; it can be
done incrementally.

Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adrian Crum
Adam Heath wrote:

> Adrian Crum wrote:
>> David and I are in agreement on the end result, and this point is where
>> we disagree. Like you, David wants all changes to be made in the branch.
>> I don't think a branch is needed. The interface extractions can be done
>> a little at a time in the trunk.
>>
>> A branch with the kind of extensive changes that are planned will open
>> up a HUGE can of worms when it is merged with the trunk. Consider that
>> this one interface extraction resulted in three bug reports - what would
>> happen if we introduced dozens of interface extractions in a single commit?
>
> I agree with you as well, this does *not* need to be done in a
> separate branch.
>
> However, what you did by doing the last step first, in trunk, is the
> wrong approach.
>
> * Adding a new interface to some base component does not break
> unreleated code(short amount of time).
> * Implementing said interface by an existing object does not break
> unreleated code(short amount of time).
> * Modifying other code to use the interface does not break unrelated
> code(takes a long time to do, and can be done incrementally by many
> people).
> * When other code no longer references concrete class, then
> remove/rename it(breaks unreleated code, but that needs to be
> announced, not just discussed).
>
> This set of changes does *not* need to be done in a hole; it can be
> done incrementally.

I was thinking of the hundreds of OFBiz users who will have to rewrite
add-ons/modifications if it was done that way. I was trying to keep
things reasonably backwards-compatible.

I'm working on the revert now. It will take a while to review and test.

-Adrian



Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Tim Ruppert
In reply to this post by Adam Heath-2
+1

On Sep 8, 2009, at 1:49 PM, Adam Heath wrote:

> Adrian Crum wrote:
>> David and I are in agreement on the end result, and this point is  
>> where
>> we disagree. Like you, David wants all changes to be made in the  
>> branch.
>> I don't think a branch is needed. The interface extractions can be  
>> done
>> a little at a time in the trunk.
>>
>> A branch with the kind of extensive changes that are planned will  
>> open
>> up a HUGE can of worms when it is merged with the trunk. Consider  
>> that
>> this one interface extraction resulted in three bug reports - what  
>> would
>> happen if we introduced dozens of interface extractions in a single  
>> commit?
>
> I agree with you as well, this does *not* need to be done in a
> separate branch.
>
> However, what you did by doing the last step first, in trunk, is the
> wrong approach.
>
> * Adding a new interface to some base component does not break
> unreleated code(short amount of time).
> * Implementing said interface by an existing object does not break
> unreleated code(short amount of time).
> * Modifying other code to use the interface does not break unrelated
> code(takes a long time to do, and can be done incrementally by many
> people).
> * When other code no longer references concrete class, then
> remove/rename it(breaks unreleated code, but that needs to be
> announced, not just discussed).
>
> This set of changes does *not* need to be done in a hole; it can be
> done incrementally.
>


smime.p7s (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
In reply to this post by Adrian Crum
Adrian Crum wrote:

> Adam Heath wrote:
>> Adrian Crum wrote:
>>> David and I are in agreement on the end result, and this point is where
>>> we disagree. Like you, David wants all changes to be made in the branch.
>>> I don't think a branch is needed. The interface extractions can be done
>>> a little at a time in the trunk.
>>>
>>> A branch with the kind of extensive changes that are planned will open
>>> up a HUGE can of worms when it is merged with the trunk. Consider that
>>> this one interface extraction resulted in three bug reports - what would
>>> happen if we introduced dozens of interface extractions in a single
>>> commit?
>>
>> I agree with you as well, this does *not* need to be done in a
>> separate branch.
>>
>> However, what you did by doing the last step first, in trunk, is the
>> wrong approach.
>>
>> * Adding a new interface to some base component does not break
>> unreleated code(short amount of time).
>> * Implementing said interface by an existing object does not break
>> unreleated code(short amount of time).
>> * Modifying other code to use the interface does not break unrelated
>> code(takes a long time to do, and can be done incrementally by many
>> people).
>> * When other code no longer references concrete class, then
>> remove/rename it(breaks unreleated code, but that needs to be
>> announced, not just discussed).
>>
>> This set of changes does *not* need to be done in a hole; it can be
>> done incrementally.
>
> I was thinking of the hundreds of OFBiz users who will have to rewrite
> add-ons/modifications if it was done that way. I was trying to keep
> things reasonably backwards-compatible.

Not possible.  You removed GenericDelegator.getGenericDelegator, so
the external code wouldn't compile anyways.

However, if there was a central factory, that used the ServiceRegistry
pattern, and framework/entity implemented that, getGenericDelegator
then called the ServiceRegistry implementation, logging a warning(from
perspective of caller) while doing so, and the base factory then
loaded the delegator thru ServiceRegistry, then existing code will
still work, you'd get a logged warning, and you'd support the new design.

Again, all without needing to do it in a branch.

And, as a bonus, the entire community can accept patches for getting
rid of GenericDelegator.getGenericDelegator.

Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adam Heath-2
In reply to this post by Adam Heath-2
Adam Heath wrote:
> I have external code(webslinger) that needs to support multiple
> versions of ofbiz(one all the way back to 512946).  This change makes
> that impossible.  I have to have multiple versions of ofbiz
> installed(pre/post this change), and compile the class once for each
> ofbiz version.

Well, based on this change, and what will eventually happen with
ExecutionContext, I've changed my build to support this.  Kinda slick
actually.

I copied the old GenericDelegator.java, new GenericDelegator.java, and
DelegatorInterface.java, into my local build system.  Modified the old
delegator, removing all comments, all method bodies, and made them all
throw UnsupportedOperationException.  Compiled these classes locally.
 Do some classpath tweaks during the build phase, and have 2 versions
of ofbiz-webslinger.jar.  I then use debian versioned dependencies to
install the correct jar.

So, I don't need this backed out right now, altho I do think it is
still the correct thing to do.  I also think an incremental approach
to ExecutionContext is also the preferred way.
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Tim Ruppert
+1 - thanks Adam - I agree whole heartedly on the incremental approach.

Cheers,
Ruppert
--
Tim Ruppert
HotWax Media
http://www.hotwaxmedia.com

o:801.649.6594
f:801.649.6595

On Sep 8, 2009, at 4:56 PM, Adam Heath wrote:

> Adam Heath wrote:
>> I have external code(webslinger) that needs to support multiple
>> versions of ofbiz(one all the way back to 512946).  This change makes
>> that impossible.  I have to have multiple versions of ofbiz
>> installed(pre/post this change), and compile the class once for each
>> ofbiz version.
>
> Well, based on this change, and what will eventually happen with
> ExecutionContext, I've changed my build to support this.  Kinda slick
> actually.
>
> I copied the old GenericDelegator.java, new GenericDelegator.java, and
> DelegatorInterface.java, into my local build system.  Modified the old
> delegator, removing all comments, all method bodies, and made them all
> throw UnsupportedOperationException.  Compiled these classes locally.
> Do some classpath tweaks during the build phase, and have 2 versions
> of ofbiz-webslinger.jar.  I then use debian versioned dependencies to
> install the correct jar.
>
> So, I don't need this backed out right now, altho I do think it is
> still the correct thing to do.  I also think an incremental approach
> to ExecutionContext is also the preferred way.


smime.p7s (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/prod

Jacques Le Roux
Administrator
+1, I agree Adam has well shown the way

Jacques

From: "Tim Ruppert" <[hidden email]>

> +1 - thanks Adam - I agree whole heartedly on the incremental approach.
>
> Cheers,
> Ruppert
> --
> Tim Ruppert
> HotWax Media
> http://www.hotwaxmedia.com
>
> o:801.649.6594
> f:801.649.6595
>
> On Sep 8, 2009, at 4:56 PM, Adam Heath wrote:
>
>> Adam Heath wrote:
>>> I have external code(webslinger) that needs to support multiple
>>> versions of ofbiz(one all the way back to 512946).  This change makes
>>> that impossible.  I have to have multiple versions of ofbiz
>>> installed(pre/post this change), and compile the class once for each
>>> ofbiz version.
>>
>> Well, based on this change, and what will eventually happen with
>> ExecutionContext, I've changed my build to support this.  Kinda slick
>> actually.
>>
>> I copied the old GenericDelegator.java, new GenericDelegator.java, and
>> DelegatorInterface.java, into my local build system.  Modified the old
>> delegator, removing all comments, all method bodies, and made them all
>> throw UnsupportedOperationException.  Compiled these classes locally.
>> Do some classpath tweaks during the build phase, and have 2 versions
>> of ofbiz-webslinger.jar.  I then use debian versioned dependencies to
>> install the correct jar.
>>
>> So, I don't need this backed out right now, altho I do think it is
>> still the correct thing to do.  I also think an incremental approach
>> to ExecutionContext is also the preferred way.
>
>

Reply | Threaded
Open this post in threaded view
|

Re: Please do a partial revert: Re: svn commit: r802567 [1/5] - in /ofbiz/trunk: applications/order/src/org/ofbiz/order/shoppingcart/ applications/product/src/org/ofbiz/shipment/packing/ applications/product/src/org/ofbiz/shipment/verify/ applications/product/src/org/ofbiz/shipment/weigh...

Adrian Crum
In reply to this post by Adam Heath-2
Adam Heath wrote:
> However, if there was a central factory, that used the ServiceRegistry
> pattern, and framework/entity implemented that, getGenericDelegator
> then called the ServiceRegistry implementation, logging a warning(from
> perspective of caller) while doing so, and the base factory then
> loaded the delegator thru ServiceRegistry, then existing code will
> still work, you'd get a logged warning, and you'd support the new design.

Are you referring to this:

http://java.sun.com/j2se/1.3/docs/guide/jar/jar.html#Service%20Provider

?

-Adrian

12