You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@sandglass-software.com> on 2014/08/25 11:51:47 UTC
Discussion: Entity Engine Redesign
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
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
Yes, I am referring to separate transactions using suspend/resume. I
thought I mentioned that. I apologize for the confusion.
Behavioral Changes
------------------
Current (simplified for clarity):
Transaction suspendedTransaction = null;
try {
suspendedTransaction = TransactionUtil.suspend();
boolean beganTransaction = false;
try {
beganTransaction = TransactionUtil.begin();
...
TransactionUtil.commit(beganTransaction);
} catch (Exception e) {
TransactionUtil.rollback(beganTransaction, "Exception thrown", e);
...
}
} finally {
if (suspendedTransaction != null) {
TransactionUtil.resume(suspendedTransaction);
}
}
...
TransactionUtil.commit(); // Commit previously suspended Tx
Proposed (simplified for clarity):
Transaction newTransaction = TransactionFactory.newTransaction();
try {
Delegator newDelegator =
newTransaction.getDelegator(delegator.getDelegatorName());
// Do stuff with newDelegator
...
newTransaction.commit();
} catch (Exception e) {
newTransaction.rollback("Exception thrown", e);
...
}
...
Transaction originalTransaction = delegator.getTransaction();
originalTransaction.commit();
As you can see, there is no need to "suspend" or "resume" a transaction.
If you want a new transaction, you just create one. All of that
complicated suspend and resume code is due to the awful ThreadLocal
transaction stack implementation.
I will get back to you on the solution to the dirty cache.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/27/2014 11:17 AM, Scott Gray wrote:
> I don't have a problem with multi-threaded transactions, I'm not sure under what situations I would use it but I'm not against it. I have a better understanding now of why you don't want to use ThreadLocal, thanks.
>
> Regarding your code simplification, I'm not sure I understand the concept your proposing. The first confusion for me is that you mention nested transactions but we don't currently support those in OFBiz (because Geronimo doesn't). Are you referring to separate transactions using suspend/resume rather than truly nested transactions? I'm going to be nervous about agreeing to anything that fundamentally changes the transaction lifecycle as it is now, I'm familiar with it and for the most part it works well.
>
> And I still have no idea how any of this would solve the dirty cache issues. I think we're in completely different spaces about the solution to the problem and I need you to expand on it further and perhaps tie that explanation into the proposed behavioural changes to the transaction lifecycle.
>
> Where we stand:
> - I get the desire for multi-threaded transactions
> - I don't get the behavioural changes to the transaction lifecycle
> - I don't get your solution to the dirty cache
>
> Regards
> Scott
>
> On 27/08/2014, at 10:28 am, Adrian Crum <ad...@sandglass-software.com> wrote:
>
>> Okay, let's start over from the beginning. This will be lengthy...
>>
>> Around 2010 I was trying various data modelling scenarios and the "ant load-demo" task was taking to long to execute, slowing down progress. So, I started experimenting with a multi-threaded data loader. I was able to get the ant task to execute in 1/10th the time using multi-threading. I mentioned my experiment on the dev mailing list and Adam Heath got interested. He said my design was similar to SEDA.
>>
>> Anyone wanting more information on SEDA can look it up, but the main point of that project that needs to be mentioned here is the concept of making multi-threading SMARTER. That confirmed a discovery I made while working on my multi-threaded data loader - more threads does not equal better performance. There was a cutoff point where more threads slowed things down.
>>
>> In that dev list conversation with Adam, I mentioned the possibility of bringing the SEDA design concepts into OFBiz. He replied it can't be done because of the current transaction API (specifically, ThreadLocal variables tying a transaction to a thread).
>>
>> Eventually, Adam brought my multi-threaded data loader idea into the project, and thanks to his work, my laptop creates the 800+ tables in less than a second, and "ant load-demo" takes a little over a minute. I think that is pretty cool and it was worth the effort.
>>
>> But we still can't multi-thread other parts of the project - because of the use of ThreadLocal variables.
>>
>> Since that discussion in 2010, others in the Java community are beginning to discover the insight SEDA delivered. Log4J 2 uses the Disruptor design pattern - an evolution of SEDA.
>>
>> Okay, why does OFBiz use ThreadLocal variables? It's a quick-and-dirty way to implement an execution context - you don't have to pass a Transaction instance to every method call.
>>
>> The funny thing is, we use the Delegator - and THAT needs to be passed to every method call. So why not have the Delegator reference the transaction it is running in and eliminate the ThreadLocal variables?
>>
>> If we can pass a Delegator instance off to other threads, then we are one step closer to a SEDA or Disruptor design pattern for OFBiz.
>>
>> Even if those design patterns aren't attractive to you, then the code simplification I demonstrated earlier should be.
>>
>> Solving the invalid entity cache problem is a side effect, not the main motivator.
>>
>> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
Smarter Multi-threading (was: Discussion: Entity Engine Redesign)
Posted by Adrian Crum <ad...@sandglass-software.com>.
On 8/27/2014 11:17 AM, Scott Gray wrote:
> I don't have a problem with multi-threaded transactions, I'm not sure under what situations I would use it but I'm not against it. I have a better understanding now of why you don't want to use ThreadLocal, thanks.
In our current design, an incoming request is assigned to a thread and
that thread executes the entire request/response lifecycle. During that
lifecycle, the thread could be spending a lot of time waiting for disk
I/O or database I/O. That is a very real possibility and not a
hypothetical one. For example...
When I created my multi-threading data loader, I knew the database was
the bottleneck, and the single-threaded data loader was spending most of
its time waiting on the database.
So, my solution was this: Have the main thread parse the various XML
files into Java objects (as it currently did), but instead of performing
operations on those Java objects (create tables, indexes, etc) put those
objects in a queue - where OTHER threads can perform operations on them.
It was a multi-threaded Provider-Consumer design and it worked well.
That is why Adam said it was a lot like SEDA. But the difference in SEDA
is in the feedback loop - where queue parameters are monitored and
adjusted dynamically to maintain liveness. That feature was not
necessary in my scenario.
Getting back to the request/response lifecycle... If we assume the
request thread is blocked waiting for slow I/O, then the potential
exists that new requests will be blocked - unless we allow more threads.
But wait! More threads might cause problems. There is a tipping point
where adding more threads will slow things down - due to thread
maintenance overhead. (In my experience, anything more than 2*CPU
threads in OFBiz will have no effect or will slow processes down.)
So, more threads are allocated to avoid blocked requests, but those new
requests are going to have horrible response times because the server is
busy taking care of the overhead of all of those threads and not doing
any real work. Users get frustrated waiting for a response and click the
refresh button - generating more requests, and the situation escalates.
SEDA solved that problem by having only one thread to service incoming
requests. The thread has only one task - drop the request in a queue.
Additional threads service the queue. A feedback loop monitors the queue
parameters and if it appears the server is becoming overloaded, new
requests are rejected, or a quick response is sent indicating a busy
server (short-circuiting the normal execution path).
Instead of having one thread per request, you have a finite number of
threads that are closely monitored and adjusted dynamically for optimum
performance.
(This is an over-simplification of the SEDA design, but it gives the
general idea.)
What does all this have to do with OFBiz? I'm not sure. The concept is
intriguing and I think there might be an application for it in OFBiz.
One area where I applied a SEDA concept to OFBiz is with the metric
feature. If a particular request URL response time exceeds a threshold,
you can respond with an alternate view - maybe a server busy message, or
a normal page but with reduced capabilities.
Getting back to Scott's question...
Let's say that 20 requests arrive at the same time or very close
together - triggering 20 threads. Okay, that means we have 20 threads
executing service engine code, entity engine code, rendering code, etc...
But we really don't need 20 threads. We could have one thread executing
service engine code and rendering code, and maybe 5 threads executing
entity engine code (where more threads are truly needed). A design like
that requires Delegator instances and the transactions they are running
in to be handed off from one thread to another.
Adrian Crum
Sandglass Software
www.sandglass-software.com
Re: Discussion: Entity Engine Redesign
Posted by Scott Gray <sc...@hotwaxmedia.com>.
I don't have a problem with multi-threaded transactions, I'm not sure under what situations I would use it but I'm not against it. I have a better understanding now of why you don't want to use ThreadLocal, thanks.
Regarding your code simplification, I'm not sure I understand the concept your proposing. The first confusion for me is that you mention nested transactions but we don't currently support those in OFBiz (because Geronimo doesn't). Are you referring to separate transactions using suspend/resume rather than truly nested transactions? I'm going to be nervous about agreeing to anything that fundamentally changes the transaction lifecycle as it is now, I'm familiar with it and for the most part it works well.
And I still have no idea how any of this would solve the dirty cache issues. I think we're in completely different spaces about the solution to the problem and I need you to expand on it further and perhaps tie that explanation into the proposed behavioural changes to the transaction lifecycle.
Where we stand:
- I get the desire for multi-threaded transactions
- I don't get the behavioural changes to the transaction lifecycle
- I don't get your solution to the dirty cache
Regards
Scott
On 27/08/2014, at 10:28 am, Adrian Crum <ad...@sandglass-software.com> wrote:
> Okay, let's start over from the beginning. This will be lengthy...
>
> Around 2010 I was trying various data modelling scenarios and the "ant load-demo" task was taking to long to execute, slowing down progress. So, I started experimenting with a multi-threaded data loader. I was able to get the ant task to execute in 1/10th the time using multi-threading. I mentioned my experiment on the dev mailing list and Adam Heath got interested. He said my design was similar to SEDA.
>
> Anyone wanting more information on SEDA can look it up, but the main point of that project that needs to be mentioned here is the concept of making multi-threading SMARTER. That confirmed a discovery I made while working on my multi-threaded data loader - more threads does not equal better performance. There was a cutoff point where more threads slowed things down.
>
> In that dev list conversation with Adam, I mentioned the possibility of bringing the SEDA design concepts into OFBiz. He replied it can't be done because of the current transaction API (specifically, ThreadLocal variables tying a transaction to a thread).
>
> Eventually, Adam brought my multi-threaded data loader idea into the project, and thanks to his work, my laptop creates the 800+ tables in less than a second, and "ant load-demo" takes a little over a minute. I think that is pretty cool and it was worth the effort.
>
> But we still can't multi-thread other parts of the project - because of the use of ThreadLocal variables.
>
> Since that discussion in 2010, others in the Java community are beginning to discover the insight SEDA delivered. Log4J 2 uses the Disruptor design pattern - an evolution of SEDA.
>
> Okay, why does OFBiz use ThreadLocal variables? It's a quick-and-dirty way to implement an execution context - you don't have to pass a Transaction instance to every method call.
>
> The funny thing is, we use the Delegator - and THAT needs to be passed to every method call. So why not have the Delegator reference the transaction it is running in and eliminate the ThreadLocal variables?
>
> If we can pass a Delegator instance off to other threads, then we are one step closer to a SEDA or Disruptor design pattern for OFBiz.
>
> Even if those design patterns aren't attractive to you, then the code simplification I demonstrated earlier should be.
>
> Solving the invalid entity cache problem is a side effect, not the main motivator.
>
> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
Okay, let's start over from the beginning. This will be lengthy...
Around 2010 I was trying various data modelling scenarios and the "ant
load-demo" task was taking to long to execute, slowing down progress.
So, I started experimenting with a multi-threaded data loader. I was
able to get the ant task to execute in 1/10th the time using
multi-threading. I mentioned my experiment on the dev mailing list and
Adam Heath got interested. He said my design was similar to SEDA.
Anyone wanting more information on SEDA can look it up, but the main
point of that project that needs to be mentioned here is the concept of
making multi-threading SMARTER. That confirmed a discovery I made while
working on my multi-threaded data loader - more threads does not equal
better performance. There was a cutoff point where more threads slowed
things down.
In that dev list conversation with Adam, I mentioned the possibility of
bringing the SEDA design concepts into OFBiz. He replied it can't be
done because of the current transaction API (specifically, ThreadLocal
variables tying a transaction to a thread).
Eventually, Adam brought my multi-threaded data loader idea into the
project, and thanks to his work, my laptop creates the 800+ tables in
less than a second, and "ant load-demo" takes a little over a minute. I
think that is pretty cool and it was worth the effort.
But we still can't multi-thread other parts of the project - because of
the use of ThreadLocal variables.
Since that discussion in 2010, others in the Java community are
beginning to discover the insight SEDA delivered. Log4J 2 uses the
Disruptor design pattern - an evolution of SEDA.
Okay, why does OFBiz use ThreadLocal variables? It's a quick-and-dirty
way to implement an execution context - you don't have to pass a
Transaction instance to every method call.
The funny thing is, we use the Delegator - and THAT needs to be passed
to every method call. So why not have the Delegator reference the
transaction it is running in and eliminate the ThreadLocal variables?
If we can pass a Delegator instance off to other threads, then we are
one step closer to a SEDA or Disruptor design pattern for OFBiz.
Even if those design patterns aren't attractive to you, then the code
simplification I demonstrated earlier should be.
Solving the invalid entity cache problem is a side effect, not the main
motivator.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
Re: Discussion: Entity Engine Redesign
Posted by Jacques Le Roux <ja...@les7arts.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
Re: Discussion: Entity Engine Redesign
Posted by Scott Gray <sc...@hotwaxmedia.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
>> 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
<ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>>
>>>>>>>
>>>>>
>>>
>
Re: Discussion: Entity Engine Redesign
Posted by Scott Gray <sc...@hotwaxmedia.com>.
Okay whatever, thanks for wasting my time with this thread.
On 27/08/2014, at 9:39 am, Adrian Crum <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>>
>>>>>>
>>>>
>>
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>
>>>>>
>>>
>
Re: Discussion: Entity Engine Redesign
Posted by Scott Gray <sc...@hotwaxmedia.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>
>>>>
>>
Re: Discussion: Entity Engine Redesign
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On Aug 27, 2014, at 9:36 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
> We're not using EJBs [...]
I just quoted the full text: the fact that we don't use EJB is not relevant because that descriptions applies to OFBiz pretty well.
Jacopo
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
We're not using EJBs and we don't need ThreadLocal variables. OFbiz has
the Delegator, and as my code fragment shows, if the framework needs to
determine what transaction is currently running, it fetches the
transaction from the Delegator.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/27/2014 7:14 AM, Jacopo Cappellato wrote:
> This is an interesting conversation and I don't have yet a strong opinion, and I am happy to discuss new ideas, but I would be careful in changing the behavior of this core part of the system because the way transactions are managed in OFBiz has proven to be good and well suited for most of the applications built in OFBiz. There are defects, some of them are described by Adrian, but I would recommend to analyze them closely and figure out if they can be fixed with the current system.
> As regards our usage of ThreadLocal variables, please see below:
>
> On Aug 27, 2014, at 3:01 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
>
>> That doesn't solve the problem with using ThreadLocal variables.
>
> this is probably not too bad and it may be inline with what other framework do; see for example this quote from the (excellent) book "Java Concurrency in Practice" by Brian Goetz:
>
> "ThreadLocal is widely used in implementing application frameworks. For example, J2EE containers associate a transaction context with an executing thread for the duration of an EJB call. This is easily implemented using a static ThreadLocal holding the transaction context: when framework code needs to determine what transaction is currently running, it fetches the transaction context from this ThreadLocal."
>
> Regards,
>
> Jacopo
>
>>
>> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>>
>>>>>
>>>
>
Re: Discussion: Entity Engine Redesign
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
This is an interesting conversation and I don't have yet a strong opinion, and I am happy to discuss new ideas, but I would be careful in changing the behavior of this core part of the system because the way transactions are managed in OFBiz has proven to be good and well suited for most of the applications built in OFBiz. There are defects, some of them are described by Adrian, but I would recommend to analyze them closely and figure out if they can be fixed with the current system.
As regards our usage of ThreadLocal variables, please see below:
On Aug 27, 2014, at 3:01 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
> That doesn't solve the problem with using ThreadLocal variables.
this is probably not too bad and it may be inline with what other framework do; see for example this quote from the (excellent) book "Java Concurrency in Practice" by Brian Goetz:
"ThreadLocal is widely used in implementing application frameworks. For example, J2EE containers associate a transaction context with an executing thread for the duration of an EJB call. This is easily implemented using a static ThreadLocal holding the transaction context: when framework code needs to determine what transaction is currently running, it fetches the transaction context from this ThreadLocal."
Regards,
Jacopo
>
> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>>
>>>>
>>
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>>
>>>
>
Re: Discussion: Entity Engine Redesign
Posted by Scott Gray <sc...@hotwaxmedia.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>>
>>
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>>
>
Re: Discussion: Entity Engine Redesign
Posted by Scott Gray <sc...@hotwaxmedia.com>.
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 <ad...@sandglass-software.com> 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 <ad...@sandglass-software.com> 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
>>
Re: Discussion: Entity Engine Redesign
Posted by Adrian Crum <ad...@sandglass-software.com>.
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 <ad...@sandglass-software.com> 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
>
Re: Discussion: Entity Engine Redesign
Posted by Scott Gray <sc...@hotwaxmedia.com>.
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 <ad...@sandglass-software.com> 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