Issue Details (XML | Word | Printable)

Key: NUCCORE-882
Type: New Feature New Feature
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Unassigned
Reporter: Stephane
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
DataNucleus Core

Allow hook for TransactionEventListener to receive beforeFlush event

Created: 25/Jul/12 10:57 PM   Updated: 04/Oct/12 05:02 PM   Resolved: 24/Aug/12 09:17 PM
Component/s: Persistence, Transactions
Affects Version/s: 3.1.0.release
Fix Version/s: 3.1.1

File Attachments: 1. Text File NUCCORE-882-cleanedup.patch (5 kB)
2. Text File patch.patch (4 kB)


Severity: Proof of Concept


 Description  « Hide
Hi,

I need ane enhancement of the TransactionEventListener to get notified before flushing the transaction.

best regards,

Stephane added a comment - 25/Jul/12 10:59 PM
Implementation suggestion

Stephane made changes - 25/Jul/12 10:59 PM
Field Original Value New Value
Attachment dntx.zip [ 11690 ]
Andy Jefferson added a comment - 26/Jul/12 08:05 AM
Thanks, but a patch ("diff -u") is what is required, since that says what has changed and is easily applied.

Andy Jefferson made changes - 26/Jul/12 08:29 AM
Summary API enhancement Allow hook for TransactionEventListener to receive beforeFlush event
Stephane added a comment - 26/Jul/12 09:21 AM
here is a patch. Hope it's the right format.

thanks,
Steph.

Stephane made changes - 26/Jul/12 09:21 AM
Attachment beforeflush.patch [ 11691 ]
Andy Jefferson added a comment - 26/Jul/12 10:54 AM
Patch doesn't apply against SVN trunk of "core" project ... in particular ObjectManagerImpl changes won't apply.

Stephane added a comment - 30/Jul/12 12:41 PM
Here is a patch for the core project.

I've made an update before creating the patch.

is it better ?

Stephane made changes - 30/Jul/12 12:41 PM
Attachment core.patch [ 11698 ]
Andy Jefferson added a comment - 31/Jul/12 11:30 AM
Stephane, it applies this time, thx.
However I need some description of what is "preListener" and "postListener"; if a user wants to register a listener then fine, but don't see why they need two to do the same thing ... one taking in an Object and the other not.

Also, when a user calls pm.flush() this likely won't be triggered as far as I can see ... since that goes via ObjectManagerImpl.flush() not via TransactionImpl, hence your callback is incorrect

Stephane added a comment - 08/Aug/12 07:09 AM
Andy,

I will certainly remove the second listener it seems to be useless for DataNucleus.

I've seen a detail in the MetaData class (core project). It contains the extensions as a Collection with a comment saying we don't need ordering so HashSet.
Is it a problem to move it as a List to maintain the order of declaration?

Andy Jefferson added a comment - 08/Aug/12 02:09 PM
A user can specify metadata <extension> in any order, and the order should mean nothing ... ordering in an XML file, or in annotations on a field mean nothing. So no, that change would make no sense, and Set has less overhead.

Stephane added a comment - 15/Aug/12 10:34 PM
Hi Andy,

In attachment you will find the DN2.patch. According to your suggestion I've removed the postListener it's useless.
I've also renamed the property to datanucleus.transactionEventListener.

Since the last post I think there are no more changes.

Let me know when this will be available.

many thanks
Stephane

Stephane made changes - 15/Aug/12 10:34 PM
Attachment DN2.patch [ 11710 ]
Andy Jefferson added a comment - 16/Aug/12 06:31 PM
This patch is using yours but cleaning it up (so it doesn't change the formatting of many lines in ObjectManagerImpl, and sticks to DN source formatting conventions). Please base all further patches on this one.

Comment on your patch :-
As said in a previous comment, if the user calls "pm.flush()" then flushStarted() will not be called (IMHO), so it is not consistent. ObjectManagerImpl.flush() is the place that will be called in that situation so you need to add code there to call all txn listener flushStarted() somehow.

Andy Jefferson made changes - 16/Aug/12 06:31 PM
Attachment NUCCORE-882-cleanedup.patch [ 11711 ]
Andy Jefferson made changes - 16/Aug/12 06:32 PM
Attachment core.patch [ 11698 ]
Andy Jefferson made changes - 16/Aug/12 06:32 PM
Attachment beforeflush.patch [ 11691 ]
Andy Jefferson added a comment - 17/Aug/12 11:21 AM - edited
Further comment :
you can already register your own txn listener as follows
((JDOPersistenceManager)pm).getExecutionContext().getTransaction().bindTransactionEventListener(myListener);
and since the user will have to use DN classes to define their own TransactionEventListener then I don't see much point in making the registration of this something done via a persistence property - would be better to add a method to JDOPersistenceManager to register a txn listener. I just added this as
http://www.datanucleus.org/servlet/jira/browse/NUCAPIJDO-39

So the user can now do
((JDOPersistenceManager)pm).addTransactionEventListener(myListener);



The only remaining thing on this issue is the "flushStarted" event which doesn't seem to do that as you have it.

Stephane added a comment - 23/Aug/12 01:50 AM
Hi Andy,

All your last recomendations are implemented in this Patch.

thanks for your support.



Stephane made changes - 23/Aug/12 01:50 AM
Attachment Patch_100%_XTE1.patch [ 11730 ]
Andy Jefferson added a comment - 23/Aug/12 08:15 AM
Stephane,

thanks, but please revise your patch so that it just shows what lines are changed. If you look at the patch it is changing pom.xml (why?), various intellij files (they aren't in SVN and don't want them), and then in ObjectManagerImpl changing many many lines ... removing a space at the end of a line? why? I need a patch that shows just the bits to change, not some reapplication of formatting (DN has its own formatting conventions already).

From what I could see in ObjectManagerImpl there is no change at all around flush() so I repeat my question ... how is a user-invoked pm.flush() being detected for this callback?

Andy Jefferson made changes - 23/Aug/12 08:19 AM
Attachment dntx.zip [ 11690 ]
Andy Jefferson made changes - 23/Aug/12 08:19 AM
Attachment DN2.patch [ 11710 ]
Stephane added a comment - 23/Aug/12 10:43 PM
Hi,

The space changes are in fact a default behaviour in IntelliJ, it unifies the layout (replacing space by tabs...)

The changes in flush are in flushInternal at lines 3788.

Here is a new patch. I made changes in a good old editor to avoid these drawbacks.

thanks Andy

Stephane

Stephane made changes - 23/Aug/12 10:43 PM
Attachment patch.patch [ 11740 ]
Stephane made changes - 23/Aug/12 10:43 PM
Attachment Patch_100%_XTE1.patch [ 11730 ]
Andy Jefferson added a comment - 24/Aug/12 09:17 PM
SVN trunk has an adaption of your patch; changed event name to transactionPreFlush() to match others. Changed some of placement to make more consistent. Look in test.jdo.general "PersistenceManagerTest" for a test and what the expectation is (currently), and a TODO to improve when the flush event is called (see also TransactionImpl) so that we minimise the commit calls to flush.

Andy Jefferson made changes - 24/Aug/12 09:17 PM
Status Open [ 1 ] Resolved [ 5 ]
Fix Version/s 3.1.1 [ 11630 ]
Resolution Fixed [ 1 ]
Andy Jefferson made changes - 28/Aug/12 09:56 AM
Status Resolved [ 5 ] Closed [ 6 ]
Andy Jefferson made changes - 04/Oct/12 05:02 PM
Component/s Persistence [ 10200 ]
Component/s JDO [ 10201 ]