[Geoserver-devel] patch review

hey,

when you have a second could you please take a look at the patch at http://jira.codehaus.org/browse/GEOS-3106.
Asking for review 'cause though I think it is what we just agreed, it touches the interfaces so a review would be handy.

Cheers,
Gabriel

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

Gabriel Roldan ha scritto:

hey,

when you have a second could you please take a look at the patch at http://jira.codehaus.org/browse/GEOS-3106.
Asking for review 'cause though I think it is what we just agreed, it touches the interfaces so a review would be handy.

Gabriel, thanks for uploading a patch for review. It looks good to me.

I was wondering if we should add a enabled() to StoreInfo as well that
checks if the store is actually reachable. This could in turn be used
in http://jira.codehaus.org/browse/GEOS-3049 to avoid failures in
WFS requests (when one ft cannot be computed properly WFS requests
that need to setup a gt-xsd parser fail during the parser config setup).

Cheers
Andrea

--
Andrea Aime
OpenGeo - http://opengeo.org
Expert service straight from the developers.

Andrea Aime wrote:

Gabriel Roldan ha scritto:

hey,

when you have a second could you please take a look at the patch at http://jira.codehaus.org/browse/GEOS-3106.
Asking for review 'cause though I think it is what we just agreed, it touches the interfaces so a review would be handy.

Gabriel, thanks for uploading a patch for review. It looks good to me.

cool, gonna commit then.

I was wondering if we should add a enabled() to StoreInfo as well that
checks if the store is actually reachable. This could in turn be used
in http://jira.codehaus.org/browse/GEOS-3049 to avoid failures in
WFS requests (when one ft cannot be computed properly WFS requests
that need to setup a gt-xsd parser fail during the parser config setup).

I'm not sure I fully understand the issue. You mean if a ft _unrelated_ to the request under execution can't be computed? if so, it shouldn't, ok. If the ft is related to the request (say a request involving more than one ft) the request should obviously fail.

By the patches at GEOS-3049 it seems you're in need for an enabled() method in FeatureTypeInfo and StoreInfo?
only concern is if it's gonna scale. Right now the enabled() derived property does not check for actual resource availability, just cascades the isEnabled() property. But if we get to force the acquisition of the DataStore/FeatureType I'm not sure how well it'll behave UI lists wise (though yeah, when it comes to scaling to thousand of layers we'll have to implement some sort of deferred list loading that plays well with paging).

So what about this: keep enabled() being just a configuration concern, no I/O nor exceptions involved, and add a reachable() property that checks the involved resource (featuretype, datastore, coverage reader) is actually accessible? This one should be used in more constrained situations (non bulk) and may avoid a lot of try{get to the resroucepool and grab the resource}catch(resource pool complains){}

does that make sense?

Cheers
Andrea

--
Gabriel Roldan
OpenGeo - http://opengeo.org
Expert service straight from the developers.

<snip>

I'm not sure I fully understand the issue. You mean if a ft _unrelated_ to the request under execution can't be computed? if so, it shouldn't, ok. If the ft is related to the request (say a request involving more than one ft) the request should obviously fail.

By the patches at GEOS-3049 it seems you're in need for an enabled() method in FeatureTypeInfo and StoreInfo?
only concern is if it's gonna scale. Right now the enabled() derived property does not check for actual resource availability, just cascades the isEnabled() property. But if we get to force the acquisition of the DataStore/FeatureType I'm not sure how well it'll behave UI lists wise (though yeah, when it comes to scaling to thousand of layers we'll have to implement some sort of deferred list loading that plays well with paging).

So what about this: keep enabled() being just a configuration concern, no I/O nor exceptions involved, and add a reachable() property that checks the involved resource (featuretype, datastore, coverage reader) is actually accessible? This one should be used in more constrained situations (non bulk) and may avoid a lot of try{get to the resroucepool and grab the resource}catch(resource pool complains){}

I think I agree with Gabriel in that adding a derived property which just used other properties is one thing, but adding a method that does a full blown resource lookup is another, and something I would like to avoid on model objects themselves.

As for the case described in GEOS-3049, I think just a better check on startup will fix that, since it involves physically removing jars from geoserver and restarting. I have not been able to reproduce a similar issue by simply brining a database down in between two requests. The behavior in that case seems reasonable, unless i am failing to reproduce the real issue.

does that make sense?

Cheers
Andrea

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Justin Deoliveira ha scritto:

<snip>

  > I think I agree with Gabriel in that adding a derived property which

just used other properties is one thing, but adding a method that does a full blown resource lookup is another, and something I would like to avoid on model objects themselves.

As for the case described in GEOS-3049, I think just a better check on startup will fix that, since it involves physically removing jars from geoserver and restarting. I have not been able to reproduce a similar issue by simply brining a database down in between two requests. The behavior in that case seems reasonable, unless i am failing to reproduce the real issue.

+1 on adding better startup checks

A very similar stack trace materializes also when a feature type fails
to compute, see http://jira.codehaus.org/browse/GEOS-3099. It would
seem the only safe approach is really to guard the xml parser
configuration code with enough try/catch to skip each type that
fails to compute, which in the particular case might be patch n1
attached to GEOS-3049

Cheers
Andrea

--
Andrea Aime
OpenGeo - http://opengeo.org
Expert service straight from the developers.