Mauro, I do not like the exception swallowing in SchemaCache.download().
You changed to original:
throw new RuntimeException(e);
to
return null;
in the catch block. While this will result in a failed download and an
exception higher up that includes a URL report, precise network or DNS
error reports will be swallowed. I am wondering if these are useful
enough for deployment debugging that they should be retained by keeping
the throw? What do you think?
My original throw was also inadequate because is prevents the reporting
of the problematic URL, causing me problems analysing this behaviour. If
you think this throw should be left, perhaps we could improve it to
report the problematic URL:
throw new RuntimeException("Error downloading " + location.toString(), e);
Your other code changes look great and your handling of the W3C licence
is just right.
Thanks!
Kind regards,
Ben.
On 23/05/13 20:07, Mauro Bartolomeoli wrote:
> Please, have a look at my fix proposal at
> https://github.com/mbarto/geotools/commit/ea85a28ff360e5be2b5c5843eb2cbf30ee68ba9f
> If you think it's ok I'll merge it on master.
>
> Thanks.
> Mauro
>
>
>
> 2013/5/23 Mauro Bartolomeoli <mauro.bartolome...@geo-solutions.it
> <mailto:mauro.bartolome...@geo-solutions.it>>
>
>
> Hi Ben,
>
>
>
> (2) Please look at the logic in SchemaCache.startDownload(). Why
> is it necessary to create an empty file? How should SchemaCache
> recover when a download fails? Please have a look and recommend
> a solution. I know you gave this some thought and I do not want
> to introduce a deadlock or thread blocking that might impact
> your performance.
>
>
> I had a look at the code, and the empty file creation seems not
> useful in that point. I don't remember why I introduced it. Maybe it
> was there for some file existance check, but that check is not there
> anymore. So I'm going to remove the empty file creation completely.
> The connect timeout instead was there to prevent a deadlock. I'm
> going to make it configurable through an env variable
> (-Dschema.cache.download.timeout is ok for you?). For the default,
> maybe we can set it to 30seconds. What do you think?
> Mauro
> --
> ==
> GeoServer training in Milan, 6th & 7th June 2013! Visit
> http://geoserver.geo-solutions.it
> <http://geoserver.geo-solutions.it/> for more information.
> ==
>
> Dott. Mauro Bartolomeoli
> @mauro_bart
> Senior Software Engineer
>
> GeoSolutions S.A.S.
> Via Poggio alle Viti 1187
> 55054 Massarosa (LU)
> Italy
> phone: +39 0584 962313 <tel:%2B39%200584%20962313>
> fax: +39 0584 1660272 <tel:%2B39%200584%201660272>
>
> http://www.geo-solutions.it
> http://twitter.com/geosolutions_it
>
> -------------------------------------------------------
>
>
>
>
> --
> ==
> GeoServer training in Milan, 6th & 7th June 2013! Visit
> http://geoserver.geo-solutions.it
> <http://geoserver.geo-solutions.it/> for more information.
> ==
>
> Dott. Mauro Bartolomeoli
> @mauro_bart
> Senior Software Engineer
>
> GeoSolutions S.A.S.
> Via Poggio alle Viti 1187
> 55054 Massarosa (LU)
> Italy
> phone: +39 0584 962313
> fax: +39 0584 1660272
>
> http://www.geo-solutions.it
> http://twitter.com/geosolutions_it
>
> -------------------------------------------------------
--
Ben Caradoc-Davies <ben.caradoc-dav...@csiro.au>
Software Engineer
CSIRO Earth Science and Resource Engineering
Australian Resources Research Centre
------------------------------------------------------------------------------
Try New Relic Now & We'll Send You this Cool Shirt
New Relic is the only SaaS-based application performance monitoring service
that delivers powerful full stack analytics. Optimize and monitor your
browser, app, & servers with just a few lines of code. Try New Relic
and get this awesome Nerd Life shirt! http://p.sf.net/sfu/newrelic_d2d_may
_______________________________________________
GeoTools-Devel mailing list
GeoTools-Devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geotools-devel
