On 01/14/2011 04:21 AM, Denis Gervalle wrote:
> Hi Caleb,
>
> We have found your proposal really interesting, and we have stated to use it
> against a true database
That is awesome! I am so glad to hear this is being used in the field, it is
too easy to write code
which looks good but in real life turns out to be difficult or impossible to
use.
> (using JDBC) for one XWiki related project we have
> in development today. I have had not much time myself to look at the
> details, but here is comments/questions from my colleague Olivier. If we
> plan to use it later for the xwiki-store, I would really appreciate your
> comments.
Absolutely, and please please tell me what is good and bad about usability, IMO
this is still fluid
enough that we can make changes to fix issues.
>
> In my understanding, for a database, I need to have a
>> StartableTransactionRunnable that roughly:
>> * Open the connection in onPreRun
Yes and acquire locks if need be.
>> * Commit the transaction in onCommit
>> * Rollback the transaction in onRollback
>> * Close the connection in onComplete
Yes and release acquired locks.
>>
>> Then, I need to have classes extending TransactionRunnable and implementing
>> the desired code in the onRun method.
>>
>> Have I correctly understood how this library should be used ?
>> If I'm correct, how do the TransactionRunnable instances retrieve the
>> connection held in the StartableTransactionRunnable ?
>> Currently I have a code similar to this:
>>
>> DBStartableTransactionRunnable run = new DBStartableTransactionRunnable
>> (dataSourceName);
>> new InsertDataTransactionRunnable(run, data).runIn(run);
>> run.start();
Yes and for safety you can define:
public interface DBTransaction { /* Empty. */ }
and
public class DBStartableTransactionRunnable extends
StartableTransactionRunnable<DBTransaction>
and
public class InsertDataTransactionRunnable extends
TransactionRunnable<DBTransaction>
thus protecting InsertDataTransactionRunnable from running outside of a
transaction.
>>
>> In which I need to pass the DBStartableTransactionRunnable instance in
>> order to be able to access the exposed Connection object which is
>> instanciated in the onPreRun method
Suppose we added this:
protected T getTransaction()
Then StartableTransactionRunnable can only be constructed by supplying an
instance of T.
So TransactionRunnable<DBTransaction>.getTransaction() would return an instance
of the DBTransaction
interface.
Then you define DBTransaction interface with a getConnection() method to get
the connection.
Does that sound workable? Should we make T be required in the constructor on in
the start()
function? Do you have a preference?
>>
>>
>> My second interrogation concern transactions that return data. What's the
>> best way to implement them ?
>> Currently what I've come up with is something like:
>>
>> DBStartableTransactionRunnable run = new DBStartableTransactionRunnable
>> (dataSourceName);
>> GetDataTransactionRunnable t = new GetDataTransactionRunnable(run, dataID)
>> t.runIn(run);
>> run.start();
>> MyData d = t.getResult();
My thinking was either: hand it an object which will be the container for the
data, or do not use
TransactionRunnable for loading since nothing is being altered and there should
be no danger of data
corruption.
>>
>>
>> I have a third interrogation, but I'm still unsure about it's validity.
>> It consist of data communication between transactions.
>> Assume that running in the same StartableTransactionRunnable we have:
>> * TR1 fetch data A1
>> * TR2 update another data A2
>> * TR3 perform some operation involving A1
>>
>> Where TR means TransactionRunnable instance. How does TR3 has access to the
>> data that is retrieved by TR1 ?
This is related to a problem which has bothered me for some time.
A TR3 can only run safely if run inside of a TR1 but we can assume that TR1 and
TR3 must both run
inside of a DBStartableTransactionRunnable. How does TR3 signal that it needs
both?
Your example has given me what I think is the answer, I will write:
public <R super P> class ProvidingTransactionRunnable<R, P> extends
TransactionRunnable<P>
R and P being "requires" and "provides". Any TransactionRunnable<P> will be
able to be run inside of
a ProvidingTransactionRunnable<R, P> and the ProvidingTransactionRunnable<R, P>
will have a modified
runIn() method allowing it to run inside of any TransactionRunnable<R>.
Now you can safely require that TR3 runs inside of TR1 by saying:
public interface MyInterfaceWithDataA1 extends DBTransaction { Data getA1(); }
public class ImplementationWithDataA1 implements MyInterfaceWithDataA1 { ... }
public class TR1 extends ProvidingTransactionRunnable<DBTransaction,
MyInterfaceWithDataA1>
{
private Data dataA1;
protected void onRun() { this.dataA1 = ..... }
/** From the proposal above, "getTransaction" is too specific, can't think
if a better term. */
@Override
protected MyInterfaceWithDataA1 getTransaction()
{
// super.getTransaction() will return a DBTransaction which our
implementation can wrap.
return new ImplementationWithDataA1(super.getTransaction(),
this.dataA1);
}
}
public class TR2 extends TransactionRunnable<DBTransaction> { ... }
public class TR3 extends TransactionRunnable<MyInterfaceWithDataA1>
{
protected void onRun()
{
// This call defers to the parent by default, TR2 can ruin the day by
// overriding getTransaction() but I don't see how this can be helped.
final Data A1 = this.getTransaction();
}
}
Then:
DBStartableTransactionRunnable run = new
DBStartableTransactionRunnable(dataSourceName);
TransactionRunnable<MyInterfaceWithDataA1> tr1 = new TR1();
tr1.runIn(run);
TransactionRunnable<DBTransaction> tr2 = new TR2();
TransactionRunnable<MyInterfaceWithDataA1> tr2WithNewCapabilities =
tr2.runIn(tr1);
new TR3().runIn(tr2WithNewCapabilities);
// This would fail at compile time: new TR3().runIn(tr2);
run.start();
This pattern will provide TR3 with it's needed data and it will make sure that
TR3 cannot be run
without the proper requirements being in place beforehand which was my original
goal.
Thank you for the feedback and for helping me solve this problem.
Please tell me about anything else you don't understand or don't like.
Caleb
>
>
> WDYT ?
> Thanks,
>
> Denis
>
> On Tue, Jan 11, 2011 at 14:50, Caleb James DeLisle <[email protected]
>> wrote:
>
>>
>>
>> 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
>>
>
>
>
_______________________________________________
devs mailing list
[email protected]
http://lists.xwiki.org/mailman/listinfo/devs
