Thanks, Mauro. That looks great! I had two small stylistic observations: (1) I do not think you need your static synchObject; you could achieve the same effect synchronising on the class object with synchronized(SchemaClass.class). (2) If you make any minor changes, please remove the trailing whitespace that appears on several lines. (I use "Show whitespace characters" in Eclipse.)
Please go ahead and merge with or without these minor changes. Kind regards, Ben. On 28/05/13 00:50, Mauro Bartolomeoli wrote: > Hi all, > > I tried to fix some remaining issues with GEOT-4467, in particular the > File.deleteOnExit() usage. > > You find the alternative solution here: > https://github.com/mbarto/geotools/commit/c1733941f9cfaf3642e7ce33cf8844b4ae21b104 > > It's based on synchronization in the topic moments of resolving from > cache, that are: > > - checking for cached schema existance on disk before downloading it > - storing the downloaded file to disk > > Synchronization is used to avoid returning a partially written file. > A check is introduced to not write a schema to disk after download, if > it already exists (downloaded from another thread). > Synchronization time should be short (the time to write a schema file on > disk). > > Let me know if this approach seems better or worse than the previous one. > > Mauro > > > > 2013/5/24 Ben Caradoc-Davies <ben.caradoc-dav...@csiro.au > <mailto:ben.caradoc-dav...@csiro.au>> > > [back on list] > > On 24/05/13 15:06, Mauro Bartolomeoli wrote: > > 2013/5/24 Ben Caradoc-Davies <ben.caradoc-dav...@csiro.au > <mailto:Ben.Caradoc-Davies@__csiro.au > <mailto:ben.caradoc-dav...@csiro.au>>> > > Mauro, I do not like the exception swallowing in > SchemaCache.download(). You changed to original: > throw new RuntimeException(e); > to > return null; > You are right, please have a look at my new commit: > > https://github.com/mbarto/__geotools/commit/__d81f9f2dbaf549c6a0b47fda96bfef__4bfccf2695 > > <https://github.com/mbarto/geotools/commit/d81f9f2dbaf549c6a0b47fda96bfef4bfccf2695> > I introduced a checked exception (IOException) so that I'm able > to catch > it and call endDownload also in case of failure (this was the > reason I > removed the throw in my first commit). > > > I don't understand. You changed this to a checked exception, but the > only place you catch the exception you just chain and throw as a > RuntimeException. Why do you need a checked exception? Unless I > missed something, the cleanup is performed in the finally block, > which is called for all exceptions, including unchecked, regardless > of the contents of the catch. Please correct me if I am wrong or > have misunderstood. > > Furthermore, if you want to use checked exceptions (which is a > matter of taste and you are quite welcome to do so if you prefer), > you can be even more consistent and catch IOException not Exception > in download(); SocketTimoutException is a sub-sub-class of > IOException. Either way should result in the same logic on timeout > or other network failure. > > I also notice that you make extensive use of File.deleteOnExit(). I > wonder if this is a best-practice as the list of files to be deleted > may grow for a long-running application. Some reports suggest it > lives in native memory. Others report deletion is unreliable. Ask > Andrea, who is our expert on performance and memory leaks. > http://puneeth.wordpress.com/__2006/01/23/filedeleteonexit-__is-evil/ > <http://puneeth.wordpress.com/2006/01/23/filedeleteonexit-is-evil/> > > http://www.pongasoft.com/blog/__yan/java/2011/05/17/file-dot-__deleteOnExit-is-evil/ > > <http://www.pongasoft.com/blog/yan/java/2011/05/17/file-dot-deleteOnExit-is-evil/> > > Oh my, I am feeling like a grumpy old man today. :-) > > > I also updated the downloadTimeout to 60 seconds (for paranoids > :-) ). > > > Looks good! > > Feel free to merge into master as it is or with minor revisions. Do > not give me a chance for more nitpicking. ;-) > > Kind regards, > > > -- > Ben Caradoc-Davies <ben.caradoc-dav...@csiro.au> > Software Engineer > CSIRO Earth Science and Resource Engineering > Australian Resources Research Centre > > > > > -- > == > 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
