So the event firing logic moved from DiffTransactionState to a
EventContentFeatureWriter wrapper.
Thanks Niels, this is a big help - and a very clear test case.
Jody Garnett
On Tue, Jul 1, 2014 at 9:04 AM, Niels Charlier <[email protected]> wrote:
> Hi Jody,
>
> I found the time to do this as volunteer. I also found some other wrong
> logic in the events system in the content data store. I made a test as you
> requested. The pull request is here:
>
> https://github.com/geotools/geotools/pull/492
>
> We can discuss the changes further if you want.
>
> Kind Regards
> Niels
>
>
> On 16/05/14 18:18, Jody Garnett wrote:
>
> Niels I would really like this fix to make it into the next release, in
> part because I am testing uDig which depends on having working events :)
>
> Is there any chance of getting a test added to this one?
>
> Jody Garnett
>
>
> On Thu, May 8, 2014 at 7:17 AM, Jody Garnett <[email protected]>
> wrote:
>
>> Unfortantly we always need a test case when accepting fixes. Still open
>> up a bug report and it can wait until one of us has time.
>>
>> Jody Garnett
>>
>>
>> On Thu, May 8, 2014 at 6:32 AM, Niels Charlier <[email protected]> wrote:
>>
>>> Hi Jody,
>>>
>>> I understand, cool.
>>> I did test that the change didn't break anything, including online tests.
>>>
>>> I wanted to report what seems to be a bug to me.
>>> Writing additional tests for this I think is out of scope of my current
>>> project.
>>> Perhaps in my own time I will do it.
>>>
>>> Kind Regards
>>> Niels
>>>
>>>
>>> On 08/05/14 05:31, Jody Garnett wrote:
>>>
>>> Your original bug report made sense, I was just going through the
>>> senario to use to test if fix works.
>>>
>>> Jody Garnett
>>>
>>>
>>> On Wed, May 7, 2014 at 10:52 AM, Niels Charlier <[email protected]> wrote:
>>>
>>>> Jody,
>>>>
>>>> If the events need to be sent to the listeners of 'source', then there
>>>> is no need for a loop. While multiply the event sent to source by the
>>>> amount of entries there are (apart from source itself)?
>>>>
>>>> If it does need to be sent to 'source', why the check if( entry ==
>>>> source ) { continue; // no notificaiton required }
>>>>
>>>> Basically, the method as it exists today could be simplified as follows:
>>>>
>>>> for(int i=0 ; i < state.values().size()-1; i++ ){
>>>>
>>>> for( FeatureListener listener : source.listeners ){
>>>> try {
>>>> listener.changed( notification );
>>>> }
>>>> catch (Throwable t ){
>>>> // problem issuing notification to an interested
>>>> party
>>>> dataStore.LOGGER.log( Level.WARNING, "Problem
>>>> issuing feature event "+notification, t );
>>>> }
>>>> }
>>>>
>>>> Now does that make sense?
>>>>
>>>> The thing is, the events have already sent to source by source itself,
>>>> it is source who calls this method after sending the events to its own
>>>> listeners. It seems to me that is why the 'continue' check is in place...
>>>> because it needs to be sent to entries *other* than source.
>>>>
>>>> Kind Regards
>>>> Niels
>>>>
>>>>
>>>>
>>>> On 07/05/14 15:32, Jody Garnett wrote:
>>>>
>>>> I am not sure Niels, we do have some tricky logic around source vs
>>>> entry, but I expect you are on to something.
>>>>
>>>> The test case to write (to confirm this) involves two transactions:
>>>> a) One transaction issuing a bulk event, such as commit
>>>> b) A second transaction, which should get this commit notification sent
>>>> to its listeners
>>>>
>>>> The other case is transaction auto commit:
>>>> a) event sent on transaction auto commit, such as feature add
>>>> b) a second transaction, which should get this feature add event
>>>> notification
>>>>
>>>> Jody
>>>>
>>>>
>>>> Jody Garnett
>>>>
>>>>
>>>> On Wed, May 7, 2014 at 7:15 AM, Niels Charlier <[email protected]> wrote:
>>>>
>>>>> Hi Everyone,
>>>>>
>>>>> while working on wfs-ng, I think I found a bug in the events system in
>>>>> gt-data.
>>>>> This is my suggested change:
>>>>>
>>>>> https://github.com/NielsCharlier/geotools/commit/af9df3e282d0ad70fbf463fa5e2b36225a53116a
>>>>>
>>>>> In
>>>>> modules/library/data/src/main/java/org/geotools/data/store/ContentEntry.java
>>>>> , now it is:
>>>>>
>>>>> for(ContentState entry : state.values() ){
>>>>> if( entry == source ) {
>>>>> continue; // no notificaiton required
>>>>> }
>>>>> for( FeatureListener listener : source.listeners ){
>>>>> try {
>>>>> listener.changed( notification );
>>>>> }
>>>>> catch (Throwable t ){
>>>>> // problem issuing notification to an interested
>>>>> party
>>>>> dataStore.LOGGER.log( Level.WARNING, "Problem
>>>>> issuing feature event "+notification, t );
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>> I would assume it should be this:
>>>>>
>>>>> for(ContentState entry : state.values() ){
>>>>> if( entry == source ) {
>>>>> continue; // no notificaiton required
>>>>> }
>>>>> for( FeatureListener listener : *entry*.listeners ){
>>>>> try {
>>>>> listener.changed( notification );
>>>>> }
>>>>> catch (Throwable t ){
>>>>> // problem issuing notification to an interested
>>>>> party
>>>>> dataStore.LOGGER.log( Level.WARNING, "Problem
>>>>> issuing feature event "+notification, t );
>>>>> }
>>>>> }
>>>>> }
>>>>>
>>>>>
>>>>> Why loop over the entries otherwise? It seems to me my change makes
>>>>> more sense.
>>>>>
>>>>> Kind Regards
>>>>> Niels
>>>>>
>>>>>
>>>>> ------------------------------------------------------------------------------
>>>>> Is your legacy SCM system holding you back? Join Perforce May 7 to
>>>>> find out:
>>>>> • 3 signs your SCM is hindering your productivity
>>>>> • Requirements for releasing software faster
>>>>> • Expert tips and advice for migrating your SCM now
>>>>> http://p.sf.net/sfu/perforce
>>>>> _______________________________________________
>>>>> GeoTools-Devel mailing list
>>>>> [email protected]
>>>>> https://lists.sourceforge.net/lists/listinfo/geotools-devel
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>
>
------------------------------------------------------------------------------
Open source business process management suite built on Java and Eclipse
Turn processes into business applications with Bonita BPM Community Edition
Quickly connect people, data, and systems into organized workflows
Winner of BOSSIE, CODIE, OW2 and Gartner awards
http://p.sf.net/sfu/Bonitasoft
_______________________________________________
GeoTools-Devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/geotools-devel
