Thank you Adrian, I truly appreciate your summary and your proposition.
I'm back after a painful week and I will hopefully cope with the hectic commits
and important conversations which happened this week.
At least I can already say I agree most of the time ThreadLocal variables are
abused.
Jacques
Le 27/08/2014 11:32, Adrian Crum a écrit :
Fair enough. Perhaps I feel like I get immediate push-back on my ideas, and I feel like no one is taking me seriously. Those feelings are my problem
and I will deal with them.
Meanwhile, I gave a very detailed reply in another post.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/27/2014 10:28 AM, Scott Gray wrote:
Okay sure but you can't see how this thread has been a bit confusing?
You've mixed two features you'd like to implement, multi-threaded transactions and an API rewrite, with an unrelated caching problem. You're also
talking about problems with when commits occur that I haven't yet been able to tie into this discussion. All of these things should be separate
conversations.
I'm trying to discuss and understand what you're proposing and then you start up on some HotWax rant. What you seem to miss here is that you're
the one shutting down discussion not Jacopo or I. Lately you seem to have decided that it's okay to write a couple of sentences proposing major
changes and then throw your arms up when anyone asks for more detail and spend the next few months complaining about how the project is dead and
won't innovate. If you don't want to fully discuss and articulate your ideas then keep them in the drawer and save us all some time.
Regards
Scott
On 27/08/2014, at 9:47 am, Adrian Crum <[email protected]>
wrote:
On 8/27/2014 9:30 AM, Scott Gray wrote:
You're yet to describe the problem with using ThreadLocal variables.
On 25/08/2014, at 10:51 am, Adrian Crum <[email protected]>
wrote:
A Delegator instance always references the transaction it is running in.
The advantage to this approach is we gain the ability to hand off Delegator instances to other threads. Other threads can even
commit/rollback a transaction:
Transaction tx = delegator.getTransaction();
tx.commit();
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/27/2014 9:43 AM, Scott Gray wrote:
Okay whatever, thanks for wasting my time with this thread.
On 27/08/2014, at 9:39 am, Adrian Crum <[email protected]>
wrote:
I have, but no one is paying attention. Perhaps if I worked for Hotwax, I could
get someone to pay attention.
I'm done.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/27/2014 9:30 AM, Scott Gray wrote:
You're yet to describe the problem with using ThreadLocal variables.
If you read the Jira issue, I point out another problem with this clunky implementation - calling "commit" doesn't really commit the
transaction. That is why we end up with invalid data in the entity cache - because developers are fooled into thinking the "commit" calls in
Delegator code actually commit the data, but they don't.
This is the only problem you've described so far and I say it has nothing to do with our transaction management, it's because the cache isn't
transaction aware. I can't understand what changes you could be suggesting that would solve that problem for the entity cache?
I've read the ticket now and realize I'm just playing Adam's role. Please
reconsider using Synchronization or XAResource.
Regards
Scott
On 27/08/2014, at 2:01 am, Adrian Crum <[email protected]>
wrote:
That doesn't solve the problem with using ThreadLocal variables.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/26/2014 11:06 PM, Scott Gray wrote:
I don't think that's an issue with our transaction handling, it's simply a
problem that the cache isn't transaction aware.
If the cache were to be transaction aware it would need to implement the XAResource interface or perhaps even the simpler Synchronization
interface and push cache entries to the global cache only upon commit or discard them on rollback. I'm loathe to suggest XAResource because
we don't implement it properly in GenericXaResource/ServiceXaWrapper/DebugXaWrapper and it breaks some transaction managers (Atomikos is the
only one I've tried). I have a strong feeling it could be implemented using the Synchronization interface without too much trouble though.
Regards
Scott
On 26/08/2014, at 9:46 pm, Adrian Crum <[email protected]>
wrote:
The concepts of "suspend" and "resume" are implemented by a ThreadLocal stack. A "suspend" pushes the current transaction on the stack, and
a "resume" pops a transaction off the stack.
If you read the Jira issue, I point out another problem with this clunky implementation - calling "commit" doesn't really commit the
transaction. That is why we end up with invalid data in the entity cache - because developers are fooled into thinking the "commit" calls in
Delegator code actually commit the data, but they don't. The transaction is committed by the first bit of code that began the transaction -
either a request event or a service invocation.
This is an arcane problem and it is difficult to describe, but I will try to
diagram it:
Request Event
Service Dispatcher
Begin Transaction (actual begin)
Begin Service
Some service logic
Delegator calls "commit" - nothing happens
Delegator puts uncommitted values in cache
More service logic
Delegator calls "commit" - nothing happens
Delegator puts uncommitted values in cache
End Service
Commit Transaction (actual commit)
Return service results to event handler
If something goes wrong in the service and the transaction is rolled back, the
uncommitted values in the cache are still there!
You really have to spend time in Entity Engine code to fully appreciate how
awful the transaction implementation really is.
My approach keeps a Transaction reference in the Delegator. Instead of calling the fake "commit", the Delegator notifies the Transaction
about changed values. The Transaction saves the changed values locally. After the transaction is committed, the Transaction instance copies
the saved values to the cache.
If you look at my previous code fragment, there will be no more "suspend" or "resume" - if you want a new transaction, you just get another
instance and use it.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/26/2014 9:02 PM, Scott Gray wrote:
Okay so I guess I don't really understand what you're suggesting, or how it really differs much from what we have now. It's also not clear
what your suggested API changes have to do with the ThreadLocal usages?
Thanks
Scott
On 26/08/2014, at 3:22 pm, Adrian Crum <[email protected]>
wrote:
Just use the Delegator factory.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/26/2014 2:43 PM, Scott Gray wrote:
Hi Adrian,
I'll probably have plenty of questions, but the first that comes to mind is: how would you use a delegator outside of a transaction with
this approach?
Thanks
Scott
On 25/08/2014, at 10:51 am, Adrian Crum <[email protected]>
wrote:
One persistent problem with the current Entity Engine implementation is the use of ThreadLocal variables in the Delegator and
Transactions. Their use makes it difficult (and sometimes impossible) to fix Entity Engine bugs. They also make it impossible to
multi-thread a Delegator instance.
Here is what I have had percolating in my head the last few months:
Transaction tx = TransactionFactory.newTransaction();
Delegator delegator = tx.getDelegator("default");
// Do stuff with delegator
Transaction nestedTx = TransactionFactory.newTransaction();
Delegator nestedDelegator = nestedTx.getDelegator("default");
// Do stuff with nestedDelegator
nestedTx.commit();
tx.commit();
A Delegator instance always references the transaction it is running in.
The advantage to this approach is we gain the ability to hand off Delegator instances to other threads. Other threads can even
commit/rollback a transaction:
Transaction tx = delegator.getTransaction();
tx.commit();
After a commit, the Delegator instance is discarded. Any attempt to use it after a commit throws an exception (the same is true with the
Transaction instance).
Another problem is Delegator localization - which also uses ThreadLocal
variables. We can localize Delegator instances like this:
Transaction tx = TransactionFactory.newTransaction();
Delegator delegator = tx.getDelegator("default", locale);
Finally, the current implementation has a caching problem:
https://issues.apache.org/jira/browse/OFBIZ-5534
With the new design, the Delegator instance, Transaction instance, and entity
cache are tightly coupled - so that problem is easy to solve.
What do you think?
--
Adrian Crum
Sandglass Software
www.sandglass-software.com
