Re: [Geotools-devel] SchemaCache timeout error
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 [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 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**4bfccf2695https://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 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 --- -- 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
Re: [Geotools-devel] SchemaCache timeout error
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
Re: [Geotools-devel] SchemaCache timeout error
On Fri, May 24, 2013 at 4:04 AM, Ben Caradoc-Davies ben.caradoc-dav...@csiro.au wrote: I think your decision to have a timeout is excellent, as a thread could otherwise block indefinitely, which would be very, very bad for a service such as GeoServer. Or would block indefinitely geotools own tests, some of which do still like to fetch for that file. Actually it's not indefinite, but some geotools modules have an unusually large build time because of that (while others have been fixed to fetch that file from the local disk). It actually seems to be site specific, on the build server gt-xsd-wfs is fast to build (5 seconds), but on my machine it takes over 40 seconds because of the schemas it's downloading from the net (one of which is, if memory serves me right, xml.xsd). Cheers Andrea -- == GeoServer training in Milan, 6th 7th June 2013! Visit http://geoserver.geo-solutions.it for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via Poggio alle Viti 1187 55054 Massarosa (LU) Italy phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it --- -- 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
Re: [Geotools-devel] SchemaCache timeout error
[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-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/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://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 -- 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
Re: [Geotools-devel] SchemaCache timeout error
On Thu, May 23, 2013 at 5:48 AM, Ben Caradoc-Davies ben.caradoc-dav...@csiro.au wrote: Mauro, your improvements to SchemaCache have also introduced an unanticipated problem triggered by download failure: https://jira.codehaus.org/browse/GEOT-4467 You set a 10 second timeout on connections and reads. It turns out that some application schemas, including that used in the GeoServer app-schema tutorial, import this W3 schema, which often takes more than 10 seconds to connect: http://www.w3.org/2001/xml.xsd Ah, this one is a nightmare, it's always super-slow to download (I guess because anything XML ends up linking it). Imho, we should have a copy of it baked directly into code to avoid downloading it over and over. (1) Why do you set a timeout at all? Just a best practice to avoid an application hang? (Good idea!) If so, the obvious workaround is to increase the timeout (60 seconds?) to accommodate slow servers. Would this be a satisfactory solution? I'd say, make it configurable. 10 seconds to start responding in 2013 is not slow, it's a geologic era... (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. Fully agree this is a problem The original AppSchemaCache had no timeout and did not write to a file until it had all the bytes of the remote resource, so 9.x is not affected by these issues. Right right, good idea to complete the write on disk only when the download is complete to avoid half downloaded files. But keeping everything in memory could be dangerous, maybe write a temp file and rename it to the final destination once done Cheers Andrea -- == GeoServer training in Milan, 6th 7th June 2013! Visit http://geoserver.geo-solutions.it for more information. == Ing. Andrea Aime @geowolf Technical Lead GeoSolutions S.A.S. Via Poggio alle Viti 1187 55054 Massarosa (LU) Italy phone: +39 0584 962313 fax: +39 0584 1660272 mob: +39 339 8844549 http://www.geo-solutions.it http://twitter.com/geosolutions_it --- -- 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
Re: [Geotools-devel] SchemaCache timeout error
On 23/05/13 14:26, Andrea Aime wrote: On Thu, May 23, 2013 at 5:48 AM, Ben Caradoc-Davies ben.caradoc-dav...@csiro.au mailto:ben.caradoc-dav...@csiro.au wrote: http://www.w3.org/2001/xml.xsd Ah, this one is a nightmare, it's always super-slow to download (I guess because anything XML ends up linking it). Imho, we should have a copy of it baked directly into code to avoid downloading it over and over. Placing this file as /org/w3/www/2001/xml.xsd on the classpath (in any jar) will cause SchemaResolver to pick it up before hitting SchemaCache. The gt-xml jar is a good place. This is currently being imported via the new xlink.xsd and smil20.xsd (since the great XLink change of 2012). (1) Why do you set a timeout at all? Just a best practice to avoid an application hang? (Good idea!) If so, the obvious workaround is to increase the timeout (60 seconds?) to accommodate slow servers. Would this be a satisfactory solution? I'd say, make it configurable. 10 seconds to start responding in 2013 is not slow, it's a geologic era... Good idea. But what default? Java 6 defaults to waiting indefinitely, which will never fail[*]. :-D [*] Depending on your definition of failure, of course. The original AppSchemaCache had no timeout and did not write to a file until it had all the bytes of the remote resource, so 9.x is not affected by these issues. Right right, good idea to complete the write on disk only when the download is complete to avoid half downloaded files. But keeping everything in memory could be dangerous, maybe write a temp file and rename it to the final destination once done The resources are only kilobytes. Using a temp file is Mauro's solution to parallel thread support; the temp copy is discarded after use (the first thread uses a non-temp location). A subtle change to this behaviour could be a solution. Kind regards, -- 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
Re: [Geotools-devel] SchemaCache timeout error
2013/5/23 Andrea Aime andrea.a...@geo-solutions.it On Thu, May 23, 2013 at 5:48 AM, Ben Caradoc-Davies ben.caradoc-dav...@csiro.au wrote: Mauro, your improvements to SchemaCache have also introduced an unanticipated problem triggered by download failure: https://jira.codehaus.org/browse/GEOT-4467 You set a 10 second timeout on connections and reads. It turns out that some application schemas, including that used in the GeoServer app-schema tutorial, import this W3 schema, which often takes more than 10 seconds to connect: http://www.w3.org/2001/xml.xsd Ah, this one is a nightmare, it's always super-slow to download (I guess because anything XML ends up linking it). Imho, we should have a copy of it baked directly into code to avoid downloading it over and over. Right! that was the schema that has triggered this work at the beginning. (1) Why do you set a timeout at all? Just a best practice to avoid an application hang? (Good idea!) If so, the obvious workaround is to increase the timeout (60 seconds?) to accommodate slow servers. Would this be a satisfactory solution? I'd say, make it configurable. 10 seconds to start responding in 2013 is not slow, it's a geologic era... How would you do that, through a -Dschema.cache.connection.timeout=60 for example ? (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. Fully agree this is a problem The original AppSchemaCache had no timeout and did not write to a file until it had all the bytes of the remote resource, so 9.x is not affected by these issues. Right right, good idea to complete the write on disk only when the download is complete to avoid half downloaded files. But keeping everything in memory could be dangerous, maybe write a temp file and rename it to the final destination once done Yes, I think using a temp file name should do the trick. Mauro -- == GeoServer training in Milan, 6th 7th June 2013! Visit 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 --- -- 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
Re: [Geotools-devel] SchemaCache timeout error
2013/5/23 Ben Caradoc-Davies ben.caradoc-dav...@csiro.au On 23/05/13 14:26, Andrea Aime wrote: On Thu, May 23, 2013 at 5:48 AM, Ben Caradoc-Davies ben.caradoc-dav...@csiro.au mailto:Ben.Caradoc-Davies@**csiro.auben.caradoc-dav...@csiro.au wrote: http://www.w3.org/2001/xml.xsd Ah, this one is a nightmare, it's always super-slow to download (I guess because anything XML ends up linking it). Imho, we should have a copy of it baked directly into code to avoid downloading it over and over. Placing this file as /org/w3/www/2001/xml.xsd on the classpath (in any jar) will cause SchemaResolver to pick it up before hitting SchemaCache. The gt-xml jar is a good place. This is currently being imported via the new xlink.xsd and smil20.xsd (since the great XLink change of 2012). Sure, and it's used also by inspire schemas. Where do you thing could be a good place to put it, the gt-xml module itself? I'm currently placing it in geoserver classes folder for my clients, but having it at the library level should be useful to anyone. (1) Why do you set a timeout at all? Just a best practice to avoid an application hang? (Good idea!) If so, the obvious workaround is to increase the timeout (60 seconds?) to accommodate slow servers. Would this be a satisfactory solution? I'd say, make it configurable. 10 seconds to start responding in 2013 is not slow, it's a geologic era... Good idea. But what default? Java 6 defaults to waiting indefinitely, which will never fail[*]. :-D [*] Depending on your definition of failure, of course. The original AppSchemaCache had no timeout and did not write to a file until it had all the bytes of the remote resource, so 9.x is not affected by these issues. Right right, good idea to complete the write on disk only when the download is complete to avoid half downloaded files. But keeping everything in memory could be dangerous, maybe write a temp file and rename it to the final destination once done The resources are only kilobytes. Using a temp file is Mauro's solution to parallel thread support; the temp copy is discarded after use (the first thread uses a non-temp location). A subtle change to this behaviour could be a solution. I think so. Mauro -- == GeoServer training in Milan, 6th 7th June 2013! Visit 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 --- -- 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
Re: [Geotools-devel] SchemaCache timeout error
On 23/05/13 15:20, Mauro Bartolomeoli wrote: 2013/5/23 Ben Caradoc-Davies ben.caradoc-dav...@csiro.au mailto:ben.caradoc-dav...@csiro.au On Thu, May 23, 2013 at 5:48 AM, Ben Caradoc-Davies ben.caradoc-dav...@csiro.au mailto:Ben.Caradoc-Davies@__csiro.au mailto:ben.caradoc-dav...@csiro.au wrote: http://www.w3.org/2001/xml.xsd Placing this file as /org/w3/www/2001/xml.xsd on the classpath (in any jar) will cause SchemaResolver to pick it up before hitting SchemaCache. The gt-xml jar is a good place. Sure, and it's used also by inspire schemas. Where do you thing could be a good place to put it, the gt-xml module itself? That sounds like the perfect place. If you download http://www.w3.org/2001/xml.xsd and then save it as: modules/library/xml/src/main/resources/org/w3/www/2001/xml.xsd and commit it on master, then Maven will bundle it into the gt-xml jar, SchemaResolver will find it on the classpath, and it will never be downloaded ever again. Because we are now shipping it with GeoTools we need to acknowledge the W3C license. Please note where you got it in the module review file: modules/library/xml/src/site/apt/review.apt Please also add the W3 license to the top-level in the project license folder: http://www.w3.org/Consortium/Legal/2002/copyright-software-20021231 We already have a copy of xml.xsd in gt-xsd-core, but it is in the wrong place for SchemaResolver; also some files have been modified to work with code (unacceptable for app-schema users). I do not know if the copyright of this file was acknowledged. Please check the review file. PMC, do we need to include the W3C short notice anywhere? Do we have a page that lists all the licenses? (Jody knows all.) Kind regards, -- 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
Re: [Geotools-devel] SchemaCache timeout error
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 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 --- -- 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
Re: [Geotools-devel] SchemaCache timeout error
Mauro, 30 seconds sounds like a generous default. Maybe even 60 seconds for the paranoid? I think your decision to have a timeout is excellent, as a thread could otherwise block indefinitely, which would be very, very bad for a service such as GeoServer. Kind regards, Ben. On 23/05/13 16:34, Mauro Bartolomeoli wrote: 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 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
Re: [Geotools-devel] SchemaCache timeout error
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
