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]
<mailto:[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]
<mailto:[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] <mailto:[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(FeatureListenerlistener:source.listeners){
try{
listener.changed(notification);
}
catch(Throwablet){
// 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] <mailto:[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(ContentStateentry:state.values()){
if(entry==source){
continue;// no notificaiton required
}
for(FeatureListenerlistener:source.listeners){
try{
listener.changed(notification);
}
catch(Throwablet){
// 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(ContentStateentry:state.values()){
if(entry==source){
continue;// no notificaiton required
}
for(FeatureListenerlistener:*entry*.listeners){
try{
listener.changed(notification);
}
catch(Throwablet){
// 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]
<mailto:[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
