Sure, go ahead and start a separate thread. Can I ask that for issues you find while hacking the community copy can you open jiras for me, as I do plan to incorporate all the changes you are making, just that I want them to go through the review process first.
Being the RESTConfig community module no more used, we can remove it and copy the modified restconfig in community. (do you think about a special name to avoid confusion? some prefix/suffix?)
I think the new restconfig has several improvements with respect to the older one, therefore we prefer to use the last one.
If we go in that direction, you can clean up the extension as you like.
Let me know and I will open a new thread to request setting up a community module.
Cheers,
Daniele
On Fri, May 8, 2009 at 6:40 PM, Justin Deoliveira <jdeolive@anonymised.com <mailto:jdeolive@anonymised.com>> wrote:
Well actually thinking about it, the old RESTConfig community module
is still there, and all the classes are there pre-change. So you may
just be able to use it.
My only hesitation is that we will end up in this same situation, a
bunch of work will be done there and it will not be ported properly
back to the extension. But since you guys are on a tight deadline we
don't have much choice.
So proceed as you see fit, either create a new community module or
use the existing RESTConfig one.
-Justin
Daniele Romagnoli wrote:
Hi,
while waiting for an improved catalog, as a quick workaround we
could set up a community module which is a "copy" of the actual
restconfig extension containing the workarounds.
(let's say, something similar to a branch, but restricted to a
single module).
Does this sounds ok?
Daniele
On Fri, May 8, 2009 at 6:16 PM, Simone Giannecchini
<simone.giannecchini@anonymised.com
<mailto:simone.giannecchini@anonymised.com>
<mailto:simone.giannecchini@anonymised.com
<mailto:simone.giannecchini@anonymised.com>>> wrote:
Ciao Justin,
please, see below...
On Fri, May 8, 2009 at 6:10 PM, Justin Deoliveira
<jdeolive@anonymised.com <mailto:jdeolive@anonymised.com>
<mailto:jdeolive@anonymised.com>> 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
<jdeolive@anonymised.com <mailto:jdeolive@anonymised.com>
<mailto:jdeolive@anonymised.com>>
>> 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
<jdeolive@anonymised.com <mailto:jdeolive@anonymised.com>
<mailto:jdeolive@anonymised.com>
>>>> <mailto:jdeolive@anonymised.com
<mailto:jdeolive@anonymised.com> <mailto:jdeolive@anonymised.com
<mailto:jdeolive@anonymised.com>>>>
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
>>>> Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>
<mailto:Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>>
>>>> <mailto:Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>
<mailto:Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>>>
>>>> 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
>>>> Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>
<mailto:Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>>
>>>>
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
>>> Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>
<mailto:Geoserver-devel@lists.sourceforge.net
<mailto:Geoserver-devel@lists.sourceforge.net>>
>>> https://lists.sourceforge.net/lists/listinfo/geoserver-devel
>>>
>
> --
> Justin Deoliveira
> OpenGeo - http://opengeo.org
> Enterprise support for open source geospatial.
>
-- -------------------------------------------------------
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
-------------------------------------------------------
-- Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.
--
-------------------------------------------------------
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
-------------------------------------------------------