[Geoserver-devel] review of patches for GSIP-31

Justin_Deoliveira · January 29, 2009, 5:05pm

Hi Ben,

I looked over the patches and they look pretty good i have to say, nice work. I have a couple of minor comments but they are pretty trivial (see below).

I was able to apply the patches and have all the unit tests pass. To be ultra confident i would like to run cite tests, which I plan to do a little later today.

Again, great job.

Some minor feedback.

* FeatureTypeInfoUtil

This seems to be a utility class which takes a FeatureTypeInfo's name and namesapce qualifies it. I think it makes sense to just have the qualified name just be a regular property on FeatureTypeInfo (maybe ResourceInfo).

* LegacyCatalogImporter

I think some of your patch rolls back some recent changes i made:

                  String defaultStyleName = ftInfoReader.defaultStyle();
                  if ( defaultStyleName != null ) {
- StyleInfo style = catalog.getStyleByName(defaultStyleName);
- if ( style != null ) {
- layer.setDefaultStyle(style);
- layer.getStyles().add(style);
- }
+ layer.setDefaultStyle(catalog.getStyleByName(defaultStyleName));
                  }

Map legendURL = ftInfoReader.legendURL();
@@ -240,11 +239,7 @@

                  String defaultStyleName = cInfoReader.defaultStyle();
                  if ( defaultStyleName != null ) {
- StyleInfo style = catalog.getStyleByName(defaultStyleName);
- if ( style != null ) {
- layer.setDefaultStyle(style);
- layer.getStyles().add(style);
- }
+ layer.setDefaultStyle(catalog.getStyleByName(defaultStyleName));
                  }
-Justin

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Rob_Atkinson1 · January 29, 2009, 10:27pm

Thanks for this work Justin,

a couple of notes,

making WMS work with a DataAccess is on the radar, keeping it working
with DataStore is the obvious short term requirement.

so, do the inbuilt unit tests or the CITE tests contain a sufficient
regression test for WMS? I got the impression it was a more manual
process, in which case we need a range of folk to do the sort of tests
they are familiar with once the patch is committed I guess. fixing any
issues will be a high priority for us too - we are not abandoning any
code here

Finally, once the encoder stuff is working I'll be motivated to
revisit the GML3.2 bindings - I confess it slipped off my radar a bit
when INPSIRE releases GML3.1.1 (for short term testing purposes) and
my own holiday time for this work evaporated.

Rob

On Fri, Jan 30, 2009 at 4:05 AM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hi Ben,

I looked over the patches and they look pretty good i have to say, nice
work. I have a couple of minor comments but they are pretty trivial (see
below).

I was able to apply the patches and have all the unit tests pass. To be
ultra confident i would like to run cite tests, which I plan to do a
little later today.

Again, great job.

Some minor feedback.

* FeatureTypeInfoUtil

This seems to be a utility class which takes a FeatureTypeInfo's name
and namesapce qualifies it. I think it makes sense to just have the
qualified name just be a regular property on FeatureTypeInfo (maybe
ResourceInfo).

* LegacyCatalogImporter

I think some of your patch rolls back some recent changes i made:

                 String defaultStyleName = ftInfoReader.defaultStyle();
                 if ( defaultStyleName != null ) {
- StyleInfo style =
catalog.getStyleByName(defaultStyleName);
- if ( style != null ) {
- layer.setDefaultStyle(style);
- layer.getStyles().add(style);
- }
+
layer.setDefaultStyle(catalog.getStyleByName(defaultStyleName));
                 }

                 Map legendURL = ftInfoReader.legendURL();
@@ -240,11 +239,7 @@

                 String defaultStyleName = cInfoReader.defaultStyle();
                 if ( defaultStyleName != null ) {
- StyleInfo style =
catalog.getStyleByName(defaultStyleName);
- if ( style != null ) {
- layer.setDefaultStyle(style);
- layer.getStyles().add(style);
- }
+
layer.setDefaultStyle(catalog.getStyleByName(defaultStyleName));
                 }
-Justin

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

------------------------------------------------------------------------------
This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

Chris_Holmes1 · January 29, 2009, 10:57pm

The best way to get some additional testing would be to roll a release. 2.0 is still alpha, so I might recommend putting out an alpha2 as soon as possible after the patches get applied. Then users can download and test.

On Thu, Jan 29, 2009 at 4:27 PM, Rob Atkinson <robatkinson101@anonymised.com> wrote:

Thanks for this work Justin,

a couple of notes,

making WMS work with a DataAccess is on the radar, keeping it working
with DataStore is the obvious short term requirement.

so, do the inbuilt unit tests or the CITE tests contain a sufficient
regression test for WMS? I got the impression it was a more manual
process, in which case we need a range of folk to do the sort of tests
they are familiar with once the patch is committed I guess. fixing any
issues will be a high priority for us too - we are not abandoning any
code here

Finally, once the encoder stuff is working I’ll be motivated to
revisit the GML3.2 bindings - I confess it slipped off my radar a bit
when INPSIRE releases GML3.1.1 (for short term testing purposes) and
my own holiday time for this work evaporated.

Rob

On Fri, Jan 30, 2009 at 4:05 AM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hi Ben,

I looked over the patches and they look pretty good i have to say, nice
work. I have a couple of minor comments but they are pretty trivial (see
below).

I was able to apply the patches and have all the unit tests pass. To be
ultra confident i would like to run cite tests, which I plan to do a
little later today.

Again, great job.

Some minor feedback.

FeatureTypeInfoUtil

This seems to be a utility class which takes a FeatureTypeInfo’s name
and namesapce qualifies it. I think it makes sense to just have the
qualified name just be a regular property on FeatureTypeInfo (maybe
ResourceInfo).

LegacyCatalogImporter

I think some of your patch rolls back some recent changes i made:

String defaultStyleName = ftInfoReader.defaultStyle();
if ( defaultStyleName != null ) {

StyleInfo style =
catalog.getStyleByName(defaultStyleName);

if ( style != null ) {

layer.setDefaultStyle(style);

layer.getStyles().add(style);

}

layer.setDefaultStyle(catalog.getStyleByName(defaultStyleName));
}

Map legendURL = ftInfoReader.legendURL();
@@ -240,11 +239,7 @@

String defaultStyleName = cInfoReader.defaultStyle();
if ( defaultStyleName != null ) {

StyleInfo style =
catalog.getStyleByName(defaultStyleName);

if ( style != null ) {

layer.setDefaultStyle(style);

layer.getStyles().add(style);

}

layer.setDefaultStyle(catalog.getStyleByName(defaultStyleName));
}
-Justin

–
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword

Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword

Geoserver-devel mailing list
Geoserver-devel@anonymised.comsts.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

Ben_Caradoc-Davies · January 30, 2009, 4:20am

Justin Deoliveira wrote:

I was able to apply the patches and have all the unit tests pass. To be ultra confident i would like to run cite tests, which I plan to do a little later today.

That would be great!

Again, great job.

Thanks.

Some minor feedback.
* FeatureTypeInfoUtil
This seems to be a utility class which takes a FeatureTypeInfo's name and namesapce qualifies it. I think it makes sense to just have the qualified name just be a regular property on FeatureTypeInfo (maybe ResourceInfo).

I did consider expanding the FeatureTypeInfo interface and its implementations, but this opens a can of worms.

The catalog has a history of supporting simple data sources with simple configuration parameters, with the implementation knowing how to do the right thing. This is a reasonable design to support the easy GUI configuration that make GeoServer popular, but as data sources become more sophisticated, the design of the catalog will have to be reviewed.

FeatureTypeInfo and friends suffer from poor separation of concerns. As well as the assumption that every data store has only one namespace, there is also a lot of nasty information smuggling going on. For example, the FeatureTypeInfo native name is documented to contain, for example, the "shapefile name" or "database table". In my view, it would be better if all mapping from feature type name to resource was handled by the data store. This would be a better separation of concerns. In the current implementation, the native name is acting as a store for some configuration information. Is this so it can be accessed by the configuration UI? Seems a bit dodgy to me. This is a class that does too much.

Given concerns about scope creep, and the need to consider the interaction between the catalog and configuration UI, I decided to make the least intrusive changes: the glue methods you find in FeatureTypeInfoUtil. I would welcome improvements to the catalog and UI, but these are outside the scope of this GSIP.

* LegacyCatalogImporter
I think some of your patch rolls back some recent changes i made:

Well spotted! My apologies, those two changes were unintentional. (Merged changes accidentally lost while editing?) Please reject or revert those chunks.

For the record, I have updated the patch attached to GEOS-2569 to remove those chunks:
http://jira.codehaus.org/browse/GEOS-2569

A unit test for correct behaviour with a null style would have caught this regression. ;->

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Justin_Deoliveira · January 30, 2009, 3:03pm

Ben Caradoc-Davies wrote:

Justin Deoliveira wrote:

I was able to apply the patches and have all the unit tests pass. To be ultra confident i would like to run cite tests, which I plan to do a little later today.

That would be great!

Again, great job.

Thanks.

Some minor feedback.
* FeatureTypeInfoUtil
This seems to be a utility class which takes a FeatureTypeInfo's name and namesapce qualifies it. I think it makes sense to just have the qualified name just be a regular property on FeatureTypeInfo (maybe ResourceInfo).

I did consider expanding the FeatureTypeInfo interface and its implementations, but this opens a can of worms.

The catalog has a history of supporting simple data sources with simple configuration parameters, with the implementation knowing how to do the right thing. This is a reasonable design to support the easy GUI configuration that make GeoServer popular, but as data sources become more sophisticated, the design of the catalog will have to be reviewed.

FeatureTypeInfo and friends suffer from poor separation of concerns. As well as the assumption that every data store has only one namespace, there is also a lot of nasty information smuggling going on. For example, the FeatureTypeInfo native name is documented to contain, for example, the "shapefile name" or "database table". In my view, it would be better if all mapping from feature type name to resource was handled by the data store. This would be a better separation of concerns. In the current implementation, the native name is acting as a store for some configuration information. Is this so it can be accessed by the configuration UI? Seems a bit dodgy to me. This is a class that does too much.

Given concerns about scope creep, and the need to consider the interaction between the catalog and configuration UI, I decided to make the least intrusive changes: the glue methods you find in FeatureTypeInfoUtil. I would welcome improvements to the catalog and UI, but these are outside the scope of this GSIP.

I agree that there is a poor separation of concerns, which is why we want to make a publishing / data split. Moving all the "service publishing metdata", wfs namespace, wms style, etc... to LayerInfo, and have ResoruceInfo just be information about the raw data. So if you want to publish a layer into two namespaces, you still have one ResourceINfo, but two LayerInfo.

I guess app-schema datastore/dataaccess is a special cause, but generally the datastore knows nothing about what namespace a resource should be published under, nor should it in my opinion.

Anyways, I would still lean towards just making this an attribute of ResourceInfo, and avoid breaking out utility classes if not necessary (there are already a too many of them in GeoServer ). And currently as it sits, it seems unnecessary.

* LegacyCatalogImporter
I think some of your patch rolls back some recent changes i made:

Well spotted! My apologies, those two changes were unintentional. (Merged changes accidentally lost while editing?) Please reject or revert those chunks.

For the record, I have updated the patch attached to GEOS-2569 to remove those chunks:
http://jira.codehaus.org/browse/GEOS-2569

A unit test for correct behaviour with a null style would have caught this regression. ;->

Yup, and if only my time was infinite we would have 100% coverage and catch all reggresions.

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Ben_Caradoc-Davies · February 3, 2009, 6:22am

Justin, please clarify the note below. Are you requesting a revision?

Justin Deoliveira wrote:

Ben Caradoc-Davies wrote:

Justin Deoliveira wrote:

* FeatureTypeInfoUtil
This seems to be a utility class which takes a FeatureTypeInfo's name and namesapce qualifies it. I think it makes sense to just have the qualified name just be a regular property on FeatureTypeInfo (maybe ResourceInfo).

I did consider expanding the FeatureTypeInfo interface and its implementations, but this opens a can of worms.

[snip]

Given concerns about scope creep, and the need to consider the interaction between the catalog and configuration UI, I decided to make the least intrusive changes: the glue methods you find in FeatureTypeInfoUtil. I would welcome improvements to the catalog and UI, but these are outside the scope of this GSIP.

I agree that there is a poor separation of concerns, which is why we want to make a publishing / data split. Moving all the "service publishing metdata", wfs namespace, wms style, etc... to LayerInfo, and have ResoruceInfo just be information about the raw data. So if you want to publish a layer into two namespaces, you still have one ResourceINfo, but two LayerInfo.

I guess app-schema datastore/dataaccess is a special cause, but generally the datastore knows nothing about what namespace a resource should be published under, nor should it in my opinion.

Anyways, I would still lean towards just making this an attribute of ResourceInfo, and avoid breaking out utility classes if not necessary (there are already a too many of them in GeoServer ). And currently as it sits, it seems unnecessary.

Please clarify if you will accept the patch as it, or whether your concerns on this point are sufficient to block acceptance of the patches.

(I am not sure if I am waiting for your review or you are waiting for me to make further changes.)

* LegacyCatalogImporter
I think some of your patch rolls back some recent changes i made:

[snip]

A unit test for correct behaviour with a null style would have caught this regression. ;->

Yup, and if only my time was infinite we would have 100% coverage and catch all reggresions.

Note winky-smirky-smiley (I am as guilty as anyone).

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Justin_Deoliveira · February 3, 2009, 3:15pm

<snip>

Please clarify if you will accept the patch as it, or whether your concerns on this point are sufficient to block acceptance of the patches.

(I am not sure if I am waiting for your review or you are waiting for me to make further changes.)

I would like to see the changes I suggested rolled in, unless you have a serious issue with them. They are pretty trivial. The only argument i have heard at the moment is they are out of scope. And some other things about separation of concerns... which does not seem too concrete. If the issue is time and scope... perhaps i can help out.

* LegacyCatalogImporter
I think some of your patch rolls back some recent changes i made:

[snip]

A unit test for correct behaviour with a null style would have caught this regression. ;->

Yup, and if only my time was infinite we would have 100% coverage and catch all reggresions.

Note winky-smirky-smiley (I am as guilty as anyone).

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Ben_Caradoc-Davies · February 5, 2009, 4:07am

Justin Deoliveira wrote:
[Get rid of FeatureTypeInfoUtil]

I would like to see the changes I suggested rolled in, unless you have a serious issue with them. They are pretty trivial. The only argument i have heard at the moment is they are out of scope. And some other things about separation of concerns... which does not seem too concrete. If the issue is time and scope... perhaps i can help out.

Justin, I have removed FeatureTypeInfoUtil and added two methods to ResourceInfo and its implementations. This is a good change; ensuring that the FeatureTypeInfo is fully constructed in LegacyCatalogImporter before the qualified name is used removes the need for the ugly hackery in FeatureTypeInfoUtil, and the implementation becomes trivial. Thanks for holding out on this one. You were right.

I have updated GSIP 31 to record the change to ResourceInfo:
http://geoserver.org/display/GEOS/GSIP+31+-+Use+DataAccess+API

I have also updated the patch for main attached to GESO-2569:
http://jira.codehaus.org/browse/GEOS-2569

*** Jira comment ***

Updated patch for main module to implement GSIP 31.

The updated patch differs from the earlier patch in that it:
(1) Removes FeatureTypeInfoUtil.
(2) Adds getQualifiedName() and getQualifiedNativeName() to ResourceInfo.
(3) Implements the methods in (2) in all implementations.
(4) Replaces the use of FeatureTypeInfoUtil with the new methods in (2).
(5) Fixes LegacyCatalogImporter so that FeatureTypeInfo namespace is configured before methods in (2) are called, removing the need for the ugly hacks in FeatureTypeInfoUtil.

******

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Justin_Deoliveira · February 6, 2009, 3:32am

Hi Ben,

Great work! Glad that my feedback was helpful. So I am pretty happy with the way things sit. So I am happy to give my +1 for the proposal.

-Justin

Justin, I have removed FeatureTypeInfoUtil and added two methods to ResourceInfo and its implementations. This is a good change; ensuring that the FeatureTypeInfo is fully constructed in LegacyCatalogImporter before the qualified name is used removes the need for the ugly hackery in FeatureTypeInfoUtil, and the implementation becomes trivial. Thanks for holding out on this one. You were right.

I have updated GSIP 31 to record the change to ResourceInfo:
http://geoserver.org/display/GEOS/GSIP+31+-+Use+DataAccess+API

I have also updated the patch for main attached to GESO-2569:
http://jira.codehaus.org/browse/GEOS-2569

*** Jira comment ***

Updated patch for main module to implement GSIP 31.

The updated patch differs from the earlier patch in that it:
(1) Removes FeatureTypeInfoUtil.
(2) Adds getQualifiedName() and getQualifiedNativeName() to ResourceInfo.
(3) Implements the methods in (2) in all implementations.
(4) Replaces the use of FeatureTypeInfoUtil with the new methods in (2).
(5) Fixes LegacyCatalogImporter so that FeatureTypeInfo namespace is configured before methods in (2) are called, removing the need for the ugly hacks in FeatureTypeInfoUtil.

******

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Ben_Caradoc-Davies · February 11, 2009, 2:11am

So, what is the next step? We have 3 committee members voting +1 for GSIP 31, and none against.
- What happens next?
- Is GSIP 31 approved?
- Shall I commit the changes or would TOPP prefer to do so?

Kind regards,
Ben.

Justin Deoliveira wrote:

Hi Ben,

Great work! Glad that my feedback was helpful. So I am pretty happy with the way things sit. So I am happy to give my +1 for the proposal.

-Justin

Justin, I have removed FeatureTypeInfoUtil and added two methods to ResourceInfo and its implementations. This is a good change; ensuring that the FeatureTypeInfo is fully constructed in LegacyCatalogImporter before the qualified name is used removes the need for the ugly hackery in FeatureTypeInfoUtil, and the implementation becomes trivial. Thanks for holding out on this one. You were right.

I have updated GSIP 31 to record the change to ResourceInfo:
http://geoserver.org/display/GEOS/GSIP+31+-+Use+DataAccess+API

I have also updated the patch for main attached to GESO-2569:
http://jira.codehaus.org/browse/GEOS-2569

*** Jira comment ***

Updated patch for main module to implement GSIP 31.

The updated patch differs from the earlier patch in that it:
(1) Removes FeatureTypeInfoUtil.
(2) Adds getQualifiedName() and getQualifiedNativeName() to ResourceInfo.
(3) Implements the methods in (2) in all implementations.
(4) Replaces the use of FeatureTypeInfoUtil with the new methods in (2).
(5) Fixes LegacyCatalogImporter so that FeatureTypeInfo namespace is configured before methods in (2) are called, removing the need for the ugly hacks in FeatureTypeInfoUtil.

******

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

jive · February 11, 2009, 4:01am

There was a set time limit for discussion on your proposal; after that time limit a vote of +0 is assumed. With this in mind your proposal seems accepted. However we did kind of stop the proposal and then re-introduce it with the blanks filled in (so you may want to check the procedure yourself and look at a calendar).

Please note that even before the vote subsides you are allowed to start committing against your proposal (this is allow for a balance between tight timelines and open collaboration). With this in mind you are free to start committing; and you should check the calendar to see if your proposal is accepted or not.

Jody
PS. Please note that often Justin likes to review things as he merges patches; they usually end up better documented as a result - so if he offers to merge your patch I recommend it

Ben Caradoc-Davies wrote:

So, what is the next step? We have 3 committee members voting +1 for GSIP 31, and none against.
- What happens next?
- Is GSIP 31 approved?
- Shall I commit the changes or would TOPP prefer to do so?

Kind regards,
Ben.

Justin Deoliveira wrote:

Hi Ben,

Great work! Glad that my feedback was helpful. So I am pretty happy with the way things sit. So I am happy to give my +1 for the proposal.

-Justin

Justin, I have removed FeatureTypeInfoUtil and added two methods to ResourceInfo and its implementations. This is a good change; ensuring that the FeatureTypeInfo is fully constructed in LegacyCatalogImporter before the qualified name is used removes the need for the ugly hackery in FeatureTypeInfoUtil, and the implementation becomes trivial. Thanks for holding out on this one. You were right.

I have updated GSIP 31 to record the change to ResourceInfo:
http://geoserver.org/display/GEOS/GSIP+31+-+Use+DataAccess+API

I have also updated the patch for main attached to GESO-2569:
http://jira.codehaus.org/browse/GEOS-2569

*** Jira comment ***

Updated patch for main module to implement GSIP 31.

The updated patch differs from the earlier patch in that it:
(1) Removes FeatureTypeInfoUtil.
(2) Adds getQualifiedName() and getQualifiedNativeName() to ResourceInfo.
(3) Implements the methods in (2) in all implementations.
(4) Replaces the use of FeatureTypeInfoUtil with the new methods in (2).
(5) Fixes LegacyCatalogImporter so that FeatureTypeInfo namespace is configured before methods in (2) are called, removing the need for the ugly hacks in FeatureTypeInfoUtil.

******

Kind regards,

Ben_Caradoc-Davies · February 11, 2009, 7:02am

Jody,

I am happy either way. I cannot find the GeoServer policy for what to do when a GSIP is approved, and the timeline. Is it

from geotools.policies import *

? That would seem to be a governance problem. The GS developer docs on GSIP process seem to peter out ...

Justin, would you like to commit the patches? (I just updated the wms one to be more GEOS-2300 friendly).

Kind regards,
Ben.

Jody Garnett wrote:

There was a set time limit for discussion on your proposal; after that time limit a vote of +0 is assumed. With this in mind your proposal seems accepted. However we did kind of stop the proposal and then re-introduce it with the blanks filled in (so you may want to check the procedure yourself and look at a calendar).

Please note that even before the vote subsides you are allowed to start committing against your proposal (this is allow for a balance between tight timelines and open collaboration). With this in mind you are free to start committing; and you should check the calendar to see if your proposal is accepted or not.

Jody
PS. Please note that often Justin likes to review things as he merges patches; they usually end up better documented as a result - so if he offers to merge your patch I recommend it

Ben Caradoc-Davies wrote:

So, what is the next step? We have 3 committee members voting +1 for GSIP 31, and none against.
- What happens next?
- Is GSIP 31 approved?
- Shall I commit the changes or would TOPP prefer to do so?

Kind regards,
Ben.

Justin Deoliveira wrote:

Hi Ben,

Great work! Glad that my feedback was helpful. So I am pretty happy with the way things sit. So I am happy to give my +1 for the proposal.

-Justin

Justin, I have removed FeatureTypeInfoUtil and added two methods to ResourceInfo and its implementations. This is a good change; ensuring that the FeatureTypeInfo is fully constructed in LegacyCatalogImporter before the qualified name is used removes the need for the ugly hackery in FeatureTypeInfoUtil, and the implementation becomes trivial. Thanks for holding out on this one. You were right.

I have updated GSIP 31 to record the change to ResourceInfo:
http://geoserver.org/display/GEOS/GSIP+31+-+Use+DataAccess+API

I have also updated the patch for main attached to GESO-2569:
http://jira.codehaus.org/browse/GEOS-2569

*** Jira comment ***

Updated patch for main module to implement GSIP 31.

The updated patch differs from the earlier patch in that it:
(1) Removes FeatureTypeInfoUtil.
(2) Adds getQualifiedName() and getQualifiedNativeName() to ResourceInfo.
(3) Implements the methods in (2) in all implementations.
(4) Replaces the use of FeatureTypeInfoUtil with the new methods in (2).
(5) Fixes LegacyCatalogImporter so that FeatureTypeInfo namespace is configured before methods in (2) are called, removing the need for the ugly hacks in FeatureTypeInfoUtil.

******

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Justin_Deoliveira · February 11, 2009, 3:25pm

Please note that even before the vote subsides you are allowed to start committing against your proposal (this is allow for a balance between tight timelines and open collaboration). With this in mind you are free to start committing; and you should check the calendar to see if your proposal is accepted or not.

Might be careful how you want to word this. If a PSC member has produced feedback that has not been addressed no commits should be made. If only positive votes have been put forth and the remaining votes have not voiced any opinions... then yes, it would be ok to start committing.

Jody
PS. Please note that often Justin likes to review things as he merges patches; they usually end up better documented as a result - so if he offers to merge your patch I recommend it

Not this time. I actually reverted the patches i had previously applied.

Ben Caradoc-Davies wrote:

So, what is the next step? We have 3 committee members voting +1 for GSIP 31, and none against.
- What happens next?
- Is GSIP 31 approved?
- Shall I commit the changes or would TOPP prefer to do so?

Kind regards,
Ben.

Justin Deoliveira wrote:

Hi Ben,

Great work! Glad that my feedback was helpful. So I am pretty happy with the way things sit. So I am happy to give my +1 for the proposal.

-Justin

Justin, I have removed FeatureTypeInfoUtil and added two methods to ResourceInfo and its implementations. This is a good change; ensuring that the FeatureTypeInfo is fully constructed in LegacyCatalogImporter before the qualified name is used removes the need for the ugly hackery in FeatureTypeInfoUtil, and the implementation becomes trivial. Thanks for holding out on this one. You were right.

I have updated GSIP 31 to record the change to ResourceInfo:
http://geoserver.org/display/GEOS/GSIP+31+-+Use+DataAccess+API

I have also updated the patch for main attached to GESO-2569:
http://jira.codehaus.org/browse/GEOS-2569

*** Jira comment ***

Updated patch for main module to implement GSIP 31.

The updated patch differs from the earlier patch in that it:
(1) Removes FeatureTypeInfoUtil.
(2) Adds getQualifiedName() and getQualifiedNativeName() to ResourceInfo.
(3) Implements the methods in (2) in all implementations.
(4) Replaces the use of FeatureTypeInfoUtil with the new methods in (2).
(5) Fixes LegacyCatalogImporter so that FeatureTypeInfo namespace is configured before methods in (2) are called, removing the need for the ugly hacks in FeatureTypeInfoUtil.

******

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Justin_Deoliveira · February 11, 2009, 3:27pm

Ben Caradoc-Davies wrote:

So, what is the next step? We have 3 committee members voting +1 for GSIP 31, and none against.
- What happens next?
- Is GSIP 31 approved?
- Shall I commit the changes or would TOPP prefer to do so?

I would say it is approved. I would say apply the patches yourself. I rolled them back in my local checkout for other development purposes. That and I would hate to take the credit for all your hard work

Kind regards,
Ben.

Justin Deoliveira wrote:

Hi Ben,

Great work! Glad that my feedback was helpful. So I am pretty happy with the way things sit. So I am happy to give my +1 for the proposal.

-Justin

Justin, I have removed FeatureTypeInfoUtil and added two methods to ResourceInfo and its implementations. This is a good change; ensuring that the FeatureTypeInfo is fully constructed in LegacyCatalogImporter before the qualified name is used removes the need for the ugly hackery in FeatureTypeInfoUtil, and the implementation becomes trivial. Thanks for holding out on this one. You were right.

I have updated GSIP 31 to record the change to ResourceInfo:
http://geoserver.org/display/GEOS/GSIP+31+-+Use+DataAccess+API

I have also updated the patch for main attached to GESO-2569:
http://jira.codehaus.org/browse/GEOS-2569

*** Jira comment ***

Updated patch for main module to implement GSIP 31.

The updated patch differs from the earlier patch in that it:
(1) Removes FeatureTypeInfoUtil.
(2) Adds getQualifiedName() and getQualifiedNativeName() to ResourceInfo.
(3) Implements the methods in (2) in all implementations.
(4) Replaces the use of FeatureTypeInfoUtil with the new methods in (2).
(5) Fixes LegacyCatalogImporter so that FeatureTypeInfo namespace is configured before methods in (2) are called, removing the need for the ugly hacks in FeatureTypeInfoUtil.

******

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Ben_Caradoc-Davies · February 12, 2009, 2:28am

Justin Deoliveira wrote:

Ben Caradoc-Davies wrote:

So, what is the next step? We have 3 committee members voting +1 for GSIP 31, and none against.
- What happens next?
- Is GSIP 31 approved?
- Shall I commit the changes or would TOPP prefer to do so?

I would say it is approved. I would say apply the patches yourself.

Thanks. I will commit the changes as soon as possible.

That and I would hate to take the credit for all your hard work

Your review resulted in better code, so any credit would not be undeserved.

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Ben_Caradoc-Davies · February 12, 2009, 2:49am

Ben Caradoc-Davies wrote:

Justin Deoliveira wrote:

Ben Caradoc-Davies wrote:

So, what is the next step? We have 3 committee members voting +1 for GSIP 31, and none against.
- What happens next?
- Is GSIP 31 approved?
- Shall I commit the changes or would TOPP prefer to do so?

I would say it is approved. I would say apply the patches yourself.

Thanks. I will commit the changes as soon as possible.

An in the interests of full disclosure, I had better mention that I will roll in a ResourcePoolTest, while nobody is watching ...

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Ben_Caradoc-Davies · February 12, 2009, 3:45am

Ben Caradoc-Davies wrote:

Justin Deoliveira wrote:

I would say it is approved. I would say apply the patches yourself.

Thanks. I will commit the changes as soon as possible.

All the patches have been applied.

wfsv (enabled in Hudson) is broken.

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Ben_Caradoc-Davies · February 12, 2009, 4:25am

Ben Caradoc-Davies wrote:

Ben Caradoc-Davies wrote:

Justin Deoliveira wrote:

I would say it is approved. I would say apply the patches yourself.

Thanks. I will commit the changes as soon as possible.

All the patches have been applied.

wfsv (enabled in Hudson) is broken.

I fixed wfsv, and local unit tests pass. I am waiting to see what Hudson makes of it ...

Are the profiles supported on Hudson documented anywhere? What do I need to reproduce a Hudson build? Clearly it is not just "mvn install".

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia

Justin_Deoliveira · February 12, 2009, 4:30am

Ben Caradoc-Davies wrote:

Ben Caradoc-Davies wrote:

Ben Caradoc-Davies wrote:

Justin Deoliveira wrote:

I would say it is approved. I would say apply the patches yourself.

Thanks. I will commit the changes as soon as possible.

All the patches have been applied.

wfsv (enabled in Hudson) is broken.

I fixed wfsv, and local unit tests pass. I am waiting to see what Hudson makes of it ...

Are the profiles supported on Hudson documented anywhere? What do I need to reproduce a Hudson build? Clearly it is not just "mvn install".

Not really... here it is however:

mvn -U -Djava.awt.headless=true -Dtest.maxHeapSize=256M -P allExtensions clean install

Ideally we can get committers access to hudson. So you can fire off builds from the user interface. But we need to talk to arne about that.

Kind regards,

--
Justin Deoliveira
OpenGeo - http://opengeo.org
Enterprise support for open source geospatial.

Ben_Caradoc-Davies · February 12, 2009, 5:25am

Justin Deoliveira wrote:

Ben Caradoc-Davies wrote:

Are the profiles supported on Hudson documented anywhere? What do I need to reproduce a Hudson build? Clearly it is not just "mvn install".

Not really... here it is however:

mvn -U -Djava.awt.headless=true -Dtest.maxHeapSize=256M -P allExtensions clean install

Thanks very much, that is everything I need to reproduce a superset of a Hudson run (except online test case fixtures).

Kind regards,

--
Ben Caradoc-Davies <Ben.Caradoc-Davies@anonymised.com>
Software Engineer, CSIRO Exploration and Mining
Australian Resources Research Centre
26 Dick Perry Ave, Kensington WA 6151, Australia