On 01/11/2011 06:03 AM, Marius Dumitru Florea wrote:
> Hi Caleb,
>
> Sounds good. Great job! One comment below,
>
> On 01/10/2011 03:15 PM, Caleb James DeLisle wrote:
>> Hi,
>> I have been working hard on filesystem attachments and I found that
>> synchronizing manual filesystem
>> transactions with automatic database transactions was a difficult job and I
>> needed a new tool to do
>> it. So I wrote what I am proposing to be the next XWiki Persistence Engine.
>>
>> I'll start off with the fun part of the proposal, I have been calling it
>> xwiki-store so far but I am
>> so excited about the capabilities of this engine that I don't think it does
>> it justice to name it
>> "store" after the place on the corner with milk and eggs. I am proposing it
>> be named "XWiki
>> Persistence Engine", the directory will be renamed xwiki-persistence, the
>> artifact name
>> xwiki-core-persistence, and the package name org.xwiki.persistence.
>> Persistence is an attribute of
>> castles, mountains and redwood trees which I think is fitting for a
>> conservatively designed storage
>> engine.
>>
>> Now a little explanation of what I'm so excited about:
>> The common and error prone way of saving things in the database is to open a
>> transaction, enter a
>> try clause, do something then commit. If we catch an exception, then we
>> rollback.
>> something like this:
>>
>> begin transaction;
>> try {
>> do something;
>> do something else;
>> commit;
>> } catch (Any exception which may occur) {
>> rollback;
>> }
>>
>> There are 3 things which can go wrong. 1 we forget to begin the transaction,
>> 2 we forget to commit
>> and 3 we do not rollback properly. What makes things worse is often the
>> database will "assume we
>> meant to..." and things will work ok most of the time which makes things
>> much worse because bugs
>> will hide very well.
>>
>> My answer to this problem is a class called TransactionRunnable. It provides
>> a set of 5 empty
>> methods to override: onPreRun(), onRun(), onCommit(), onRollback(), and
>> onComplete(). the exact
>> circumstances under which each are called is documented in the javadoc
>> comments here:
>> http://svn.xwiki.org/svnroot/xwiki/contrib/sandbox/xwiki-store/xwiki-store-transaction/src/main/java/org/xwiki/store/TransactionRunnable.java
>> I wrote TransactionRunnable twice, I wrote it, used it for attachments, then
>> after having real
>> experience as a user, I wrote it again.
>>
>> To repeat our original example with TransactionRunnable you might say this:
>>
>> public class DoSomethingTransactionRunnable extends TransactionRunnable
>> {
>> public void onRun()
>> {
>> do something;
>> do something else;
>> }
>> }
>>
>> Now we can use another TransactionRunnable which opens and closes the
>> transaction for us.
>>
>> StartableTransactionRunnable transaction = new
>> HibernateTransactionRunnable();
>> new DoSomethingTransactionRunnable().runIn(transaction);
>> transaction.start();
>>
>> the runIn() function allows us to run one TransactionRunnable inside of
>> another. Supposing we wanted
>> to reuse "do something else" in other places, we can make it a separate
>> TransactionRunnable and use
>> the runIn() function to hook it to our DoSomethingTransactionRunnable ie:
>>
>> public class DoSomethingTransactionRunnable extends TransactionRunnable
>> {
>> public DoSomethingTransactionRunnable()
>> {
>> new DoSomethingElseTransactionRunnable().runIn(this);
>> }
>> ..
>>
>> The only limitations on running TransactionRunnables inside of one another
>> are they cannot run more
>> than once and they cannot call themselves (this would be an infinite loop).
>>
>> This pattern makes each job which is done on storage easily isolated and, as
>> I have so far
>> experienced, trivial to test. However, it still leaves the possibility that
>> we might forget that
>> DoSomethingTransactionRunnable must be run inside of a hibernate
>> transaction. I have devised a
>> solution for this too. Using generics, I offered a means for the author of a
>> TransactionRunnable to
>> communicate to the compiler what other TransactionRunnable their runnable
>> must be run in and without
>> explicit casting or defining of an intermediary runnable, this requirement
>> cannot be violated or
>> else it wouldn't compile!
>
> Is this generic enough to allow us to easily test a TransactionRunnable
> that has explicitly specified the runnable it must be run in?
>
> When you say "generics" I guess you are referring to:
>
> public class DoSomethingTransactionRunnable extends
> TransactionRunnable<HibernateTransactionRunnable>
> {
> ...
> }
>
> From what I understood HibernateTransactionRunnable must be a concrete
> class so that you can instantiate it. Then it's not easy/elegant to mock it.
Since runIn() takes a TransactionRunnable<? extends T> instead of T itself,
there are 2 options:
1. Do as you say and then DoSomethingTransactionRunnable may be runIn() any
TransactionRunnable<HibernateTransactionRunnable> so you need not mock
HibernateTransactionRunnable,
only define a TestTransactionRunnable which extends
TransactionRunnable<HibernateTransactionRunnable>
or as I prefer:
2. Create an empty interface called HibernateTransaction which exists for the
sole purpose of being
in the generic. Thus to define a
StartableTransactionRunnable<HibernateTransaction> is to promise
that you have indeed started and stopped the hibernate transaction in your
runnable.
I did spend the better part of a day trying to make sure that an evil
StartableTransactionRunnable
could not make promises that it couldn't keep but in the end I decided that
this method was better.
In regards to mocking, I have not found a need to mock anything since I can
just instantiate an
anonymous class extending TransactionRunnable, put my test logic in that, and
run my tested runnable
inside of it.
Along a similar line, you might ask why TransactionRunnable itself is not an
interface. I tried to
do this and found that first, there were a number of private methods used for
chaining which had to
be made public and was less than pleasant although acceptable. Second, runIn()
relies on access to a
private field to attach the runnable to it's parent and I couldn't face the
thought of a public
unsafeAdd() method which said "please don't use this" in the javadoc comment. I
could not think of
any use case for another implementation of the base TransactionRunnable class
and it made no sense
to me to sacrifice the outward simplicity and stability of TransactionRunnable
for something which I
could not imagine any use for.
Caleb
>
>>
>> Finally we have the issue of starting the runnable. Who's to say I won't be
>> tired one day and just
>> write new DoSomethingTransactionRunnable().start() without opening a
>> transaction first? If
>> DoSomethingTransactionRunnable cannot be safely run outside of a transaction
>> all it needs to do is
>> not extend StartableTransactionRunnable and it won't have any start function.
>>
>> I have taken a multitude of very easy mistakes and given the author of a
>> TransactionRunnable the
>> tools to make it very hard for the user to make them. Also, since a
>> TransactionRunnable has no
>> reason to be very long (it can just branch off into another runnable) this
>> will make testing and
>> code review easy in the place where it is most important. This part of the
>> code is entirely generic
>> and has no dependence on hibernate or anything else.
>>
>> I propose we move:
>> contrib/sandbox/xwiki-store/xwiki-store-transaction/
>> to:
>> platform/core/xwiki-persistence/xwiki-persistence-transaction
>>
>> And I will propose moving each additional piece in the coming days.
>>
>> WDYT?
>
> +1
>
> Thanks,
> Marius
>
>>
>> Caleb
>>
>> _______________________________________________
>> devs mailing list
>> [email protected]
>> http://lists.xwiki.org/mailman/listinfo/devs
> _______________________________________________
> devs mailing list
> [email protected]
> http://lists.xwiki.org/mailman/listinfo/devs
>
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
