Posted by
Adam Heath-2 on
Mar 11, 2010; 4:51pm
URL: http://ofbiz.116.s1.nabble.com/Re-svn-commit-r918926-in-ofbiz-site-images-follow-us-b-png-index-html-tp1578166p1589277.html
Adrian Crum wrote:
> Adam Heath wrote:
>> Moving the caching higher implies that the constructed delegator won't
>> be saved until it is completely done being constructed. However,
>> during construction, the delegator creates several helper entities.
>> These include EntityCrypto, and the cache support classes. These
>> classes also still have the same problem of not storing delegator
>> instances, instead just storing a name. So, they try to load a
>> delegator with the correct name, and don't find it, because the
>> original delegator is not done being constructed.
>>
>> But, we still aren't done with this particular problem.
>>
>> When GenericPK and GenericValue get created, they try to set their
>> fields. Setting any field on a GenericEntity requires that it knows
>> it's delegator. However, creation doesn't actually set the delegator.
>> Even the delegator name is null. So any GenericEntity created during
>> delegator instantiation ends up trying to load a delegator named
>> "default", *not* with whatever delegator is actually being created.
>
> I ran into that circular logic problem in the ExecutionContext branch. I
> agree - there are a lot of problems with the delegator implementation
> and they should be fixed. Let me know if there is anything I can do to
> help.
Oh, heh, funny you should say that.
Your DelegatorFactory change gave me many problems. It's not
backwards compatible.
Webslinger is a purely standalone framework. It's made to run in
anything, with anything. It just happens to have a way to function
alongside ofbiz.
The issue here, is that I have to produce updates to the webslinger
codebase, and make them work against multiple ofbiz versions. I
support 595296 thru 902021. Making it work against the latest
version, with the delegator interface/class changes, was rather
complex. I had to copy the old GenericDelegator class, erasing all
documentation, all code, make every method throw an
UnsupportedOperationException. I did this so I would have an
api-compatible version, that I could compile against. I then had to
make my build system compile 2 versions of webslinger, one against the
latest ofbiz, and one against an old ofbiz with this special dummy
GenericDelegator class earlier in the classpath.
When making changes to ofbiz, it's not just the internal project code
that has to be made consistent. Ofbiz doesn't exist by itself. It
has to deal with external addons, that aren't even known. As such,
much care needs to be taken any time you change the abi(not api).
This was not done with the DelegatorFactory change. I even mentioned
this briefly during it's inception. But I never spoke up in detail
about it at the time.
Also, any time a class changes(I'm saying this again, but it bears
repeating), you need to consider the use cases of it. This paragraph
is talking about the moving of some classes from util to lang, with no
attention paid to backwards compatiblity, nor deprecation. If an end
developer has to maintain multiple versions of their product against
multiple versions of ofbiz, then changing abi like this causes *more*
work for end developers.
Any time you change something, and someone finds a problem with it,
don't take the defensive, and say "How dare you find faults with my
work; I'm not going to fix this, as it's just more work for me to do."
In actuallity, you have caused more work for the person who found the
problem. You have broken their own software; you have made them
dislike the project as a whole.
Absolutely be defensive in your programming. Whenever you do
something, say to yourself: "How could this be used? Is it perfect?
Will it break things for other people, thereby causing them more work?"
And, when someone *does* find something wrong, don't respond saying
that the finder should fix the issue. *You* are the one who did the
original change, and are the absolutely bestest person to understand
what was being attempted. You are the perfect person to fix the
actual problem.
As for real things to deal with:
This applies to anyone reading this. Read that Java Concurrency in
Practice book *again(, cover to cover. All constructors must *only*
set variables internal to their *direct* class. Do *not* construct
other classes, passing 'this' to them, because they may register
themselves in global static containers. This means that 'this' is
available for use by other parts of the system, before 'this' is
actually finished being constructed.