Ciao Justin, please, see below...
On Fri, May 8, 2009 at 6:10 PM, Justin Deoliveira <[email protected]> wrote: > > > Simone Giannecchini wrote: >> >> Actually, >> yeah, I have one objection. >> Beside the fact that I am not so convinced that separating the two >> calls will make the api cleaner, > > I would argue it does from a REST perspective. Any parameters that modify > the resource being PUT should be in the request entity, not in the URI that > defines the resource. > > I am quite worried that this would >> >> make the whole process much weaker. In the real world, when you want >> to add data in a dynamic way with a heavily loaded server with the >> current geoserver internal catalog it is not uncommon that everything >> breaks due to the fact that it takes quite some time to persiste the >> changes. You understand that starting to explode the number of calls >> to configure a single layer will not work in real use cases. > > I understand the issue, but I hope you can understand why I am hesitant to > make API compromises to get around limitations which are currently just an > implementation detail. Like if we had a catalog that performed well under > update this would be an non issue. > I agree, but we don't have such a catalog yet :-). > Even in 2.0 this should be much less of an issue since now the only thing > that gets saved is what you modify. Should be much more performant under > heavy update load. That said I do realize that you are working with 1.7.x > and need to work stable. > > Putting the issue of the current catalog aside for the moment, I would > speculate that the cost of a second call should be negligible compared to > the first. The reason being that since the first request involves a data > upload it could potentially take a while assuming a decently sized dataset. > But the second call should always be constant time. So you have 1 > potentially long request + 1 constant time (instantaneous when we have a > decent catalog). > I am talking about a different thing. If you have 2000 coveragerages configured it takes a non negligible time to persist the config. Do it twice is simply not acceptable. > That said, I would like to come up with a compromise here, and I would like > to do so in a way that does not prevent you from meeting any deadlines. > Since the rest system is pluggable can we move the code that contains these > workarounds for our current catalog limitations into a community module. > Doing so allows us to have a way to achieve the desired functionality for > now, but at the same time does not tie us to it for future versions. And > with a little spring magic we should be able to do in a way that the > community module version overrides the extension version so there are no URI > conflicts. I'll live that to daniele, since we already modifying code that worked before 1.7.3, therefore there is not much time available for more work. Simone. > > Thoughts? >> >> >> Simone. >> ------------------------------------------------------- >> Ing. Simone Giannecchini >> GeoSolutions S.A.S. >> Owner - Software Engineer >> Via Carignoni 51 >> 55041 Camaiore (LU) >> Italy >> >> phone: +39 0584983027 >> fax: +39 0584983027 >> mob: +39 333 8128928 >> >> >> http://www.geo-solutions.it >> http://simboss.blogspot.com/ >> http://www.linkedin.com/in/simonegiannecchini >> >> ------------------------------------------------------- >> >> >> >> On Fri, May 8, 2009 at 5:25 PM, Justin Deoliveira <[email protected]> >> wrote: >>> >>> For the first patch: >>> >>> + final LayerInfo layerInfo; >>> + final Map<String,String> layerProperties = new >>> HashMap<String,String>(2); >>> + if (form.getFirst("style") != null) >>> + layerProperties.put("style", >>> form.getFirstValue("style")); >>> + if (form.getFirst("wmspath") != null) >>> + layerProperties.put("path", >>> form.getFirstValue("wmspath")); >>> + if (layerProperties.isEmpty()) >>> + layerInfo = builder.buildLayer(cinfo); >>> + else >>> + layerInfo = >>> builder.buildLayer(cinfo,layerProperties); >>> + catalog.add(layerInfo); >>> >>> I think this should be handled with a second call, rather than stack up >>> available options. It makes the api cleaner imo. And is less arbitrary. >>> For instance why do we support style and wms path but not the other >>> properties? >>> >>> So I would prefer the interaction to be: >>> >>> 1) PUT to upload the coverage >>> 2) PUT to modify the coverage >>> >>> Objections? >>> >>> Daniele Romagnoli wrote: >>>> >>>> Hi Justin, >>>> I'm really sorry about this. >>>> Next time I'll follows the steps you are talking about. >>>> I was working on the coverages ingestion and I needed to re-enable some >>>> capabilities available in 1.7.2 for an urgent application. >>>> I was too hasty. Sorry again. >>>> >>>> >>>> On Fri, May 8, 2009 at 4:22 PM, Justin Deoliveira <[email protected] >>>> <mailto:[email protected]>> wrote: >>>> >>>> Reviewing the commits more closely I definitely have a few concerns >>>> which affirms my preference to have these changes reviewed before >>>> hand. >>>> >>>> * The method added to catalog builder seems unnecessary to me. I >>>> think >>>> just the plain buildLayer can be called, and then properties >>>> overrideen >>>> after the fact, instead put putting them into a map and passing it >>>> into >>>> the method. It also does not sync up with any of the other methods in >>>> that class. I would like to keep that class as consistent as >>>> possible. >>>> >>>> * You have made changes to api without any discussion. Supporting >>>> wmspath and and style as query parameters is not documented as api >>>> options: >>>> >>>> >>>> http://gridlock.openplans.org/geoserver/1.7.x/doc/user/extensions/rest/rest-config-api.html#coverage-stores >>>> >>>> >>>> The previous RESTConfig allowed to handle them. I had tought during >>>> migration to extensions they have been forgotten. >>>> >>>> >>>> >>>> <http://gridlock.openplans.org/geoserver/1.7.x/doc/user/extensions/rest/rest-config-api.html#coverage-stores> >>>> >>>> Not saying we should not have them, but this is an extension now, not >>>> a >>>> community module anymore, such things require discussion before hand. >>>> >>>> >>>> You are Right. >>>> >>>> >>>> >>>> * Many of the changes seem a bit arbitrary, changing variable >>>> declarations to final for instance. Again, for consistency sake I >>>> prefer >>>> that you delegate to how the rest of the class / method does things >>>> when >>>> making chagnes. >>>> >>>> * Setting the layer name as the entity is again an api change, and >>>> one I >>>> don't agree with. It is more RESTful to set the location of the >>>> resource >>>> using the appropriate HTTP header, 'Location' like we do for the POST >>>> case. Also, the indication of where the resource should be down with >>>> a >>>> full uri, just returning the name assumes that URI's are non-opaque, >>>> ie >>>> the client has pre-knowledge of how URI's are built, which is another >>>> REST nono. >>>> >>>> >>>> Do you think it would be better if I revert my changes? >>>> Daniele >>>> >>>> >>>> >>>> >>>> >>>> -Justin >>>> >>>> Justin Deoliveira wrote: >>>> > Hi Daniele, >>>> > >>>> > I see you are making commits on 1.7.x to restconfig and main. I >>>> would >>>> > appreciate posting patches for pre review first before committing. >>>> > Especially in this case where we have not yet wrapped up >>>> discussion >>>> > about how to handle the PUT issue from previous email. >>>> > >>>> > I am not trying to hamper work here but I have been deemed the >>>> > maintainer of that extension, and while there are no hard rules >>>> in place >>>> > about who is allowed to commit, i would appreciate being to review >>>> > commits before they go through. >>>> > >>>> > -Justin >>>> > >>>> >>>> >>>> -- >>>> Justin Deoliveira >>>> OpenGeo - http://opengeo.org >>>> Enterprise support for open source geospatial. >>>> >>>> >>>> ------------------------------------------------------------------------------ >>>> The NEW KODAK i700 Series Scanners deliver under ANY circumstances! >>>> Your >>>> production scanning environment may not be a perfect world - but >>>> thanks to >>>> Kodak, there's a perfect scanner to get the job done! With the NEW >>>> KODAK i700 >>>> Series Scanner you'll get full speed at 300 dpi even with all image >>>> processing features enabled. http://p.sf.net/sfu/kodak-com >>>> _______________________________________________ >>>> Geoserver-devel mailing list >>>> [email protected] >>>> <mailto:[email protected]> >>>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel >>>> >>>> >>>> >>>> >>>> -- >>>> ------------------------------------------------------- >>>> Eng. Daniele Romagnoli >>>> Software Engineer >>>> >>>> GeoSolutions S.A.S. >>>> Via Carignoni 51 >>>> 55041 Camaiore (LU) >>>> Italy >>>> >>>> phone: +39 0584983027 >>>> fax: +39 0584983027 >>>> mob: +39 328 0559267 >>>> >>>> >>>> http://www.geo-solutions.it >>>> >>>> ------------------------------------------------------- >>>> >>>> >>>> ------------------------------------------------------------------------ >>>> >>>> >>>> ------------------------------------------------------------------------------ >>>> The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your >>>> production scanning environment may not be a perfect world - but thanks >>>> to >>>> Kodak, there's a perfect scanner to get the job done! With the NEW KODAK >>>> i700 >>>> Series Scanner you'll get full speed at 300 dpi even with all image >>>> processing features enabled. http://p.sf.net/sfu/kodak-com >>>> >>>> >>>> ------------------------------------------------------------------------ >>>> >>>> _______________________________________________ >>>> Geoserver-devel mailing list >>>> [email protected] >>>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel >>> >>> -- >>> Justin Deoliveira >>> OpenGeo - http://opengeo.org >>> Enterprise support for open source geospatial. >>> >>> >>> ------------------------------------------------------------------------------ >>> The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your >>> production scanning environment may not be a perfect world - but thanks >>> to >>> Kodak, there's a perfect scanner to get the job done! With the NEW KODAK >>> i700 >>> Series Scanner you'll get full speed at 300 dpi even with all image >>> processing features enabled. http://p.sf.net/sfu/kodak-com >>> _______________________________________________ >>> Geoserver-devel mailing list >>> [email protected] >>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel >>> > > > -- > Justin Deoliveira > OpenGeo - http://opengeo.org > Enterprise support for open source geospatial. > ------------------------------------------------------------------------------ The NEW KODAK i700 Series Scanners deliver under ANY circumstances! Your production scanning environment may not be a perfect world - but thanks to Kodak, there's a perfect scanner to get the job done! With the NEW KODAK i700 Series Scanner you'll get full speed at 300 dpi even with all image processing features enabled. http://p.sf.net/sfu/kodak-com _______________________________________________ Geoserver-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/geoserver-devel
