[Geoserver-devel] catalog dao split patch review #2

Justin_Deoliveira · February 25, 2010, 2:37pm

Hi Emanuele,

I just reviewed the updated patch, and things are getting closer.

I am worried however that behavior changes are creeping in here. I thought we agreed this would issue in particular would be a 100% refactor, ie no behavior changes. I realize that things will need to change but I think we should get the new api on first, and tehn worry about the little details of things that have to change. I fear that if we don't stick to that this change will be riddled with regressions.

I also saw that you just posted a new patch to some of this might be irrelevant.

Here is what I found this time around:

general stuff
-------------

* I see a break of code conventions with some if statements:

if(LOGGER.isLoggable(Level.FINER))
LOGGER.finer("AUTO DEFAULT NAMESPACE: " + namespace);

Please ensure you always use curly braces. Also the logging guard in this case seems rather unnecessary, although I know it is just habit.

* I see a lot of commented out code. Just remove it, it makes the patch cleaner to review.

CatalogDAO
----------

* Still no javadocs

* needsIdRemapping() ? What is the purpose of this method. Seems to expose implementation details uncessarily. Some explanation is required.

CatalogImpl
-----------

* add(WorkspaceInfo):

- Yes validate already contains this check, the check can probably be removed

* setDefaultWorkspace(WorkspaceInfo)

- Remove the check, the contract of setDefaultWorkspace states that the ws does not need to be previously added to the catalog

* postSave(CatatlogInfo)

- Why is the call to proxy.commit() commented out?

* CatalogSync

- Why a separate class for this? Why not just keep it as Catalog.sync() and just call through to the dao.

MemoryCatalogDAO
----------------

* The save() methods seems to duplicate each other. The old catalog factored this out into a common method. Again a pure refactor would have maintained this I would think.

* getLayersByRsource():

- Some checks seems to be commented out? Were these checks added by you, I actually could not find them in the original CatalogImpl class.

-Justin

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

Simone_Giannecchini3 · February 25, 2010, 2:54pm

On Thu, Feb 25, 2010 at 3:37 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Hi Emanuele,

I just reviewed the updated patch, and things are getting closer.

I am worried however that behavior changes are creeping in here. I
thought we agreed this would issue in particular would be a 100%
refactor, ie no behavior changes. I realize that things will need to
change but I think we should get the new api on first, and tehn worry
about the little details of things that have to change. I fear that if
we don't stick to that this change will be riddled with regressions.

I also saw that you just posted a new patch to some of this might be
irrelevant.

Here is what I found this time around:

general stuff
-------------

* I see a break of code conventions with some if statements:
   if\(LOGGER\.isLoggable\(Level\.FINER\)\)
            LOGGER\.finer\(&quot;AUTO DEFAULT NAMESPACE: &quot; \+ namespace\);
Please ensure you always use curly braces. Also the logging guard in
this case seems rather unnecessary, although I know it is just habit.

Logging guard is a good habit and also a requirement I have put
forward for our (GeoSolutions) code.
There are plenty of measurements around the web that shows why we
should use it, especially with logging levels like FINE, FINER, etc..
Bottom line is, I would find it hard to ask ETj to not use this, since
it is usually easy to forget good habits :-).

Ciao,
Simone.

* I see a lot of commented out code. Just remove it, it makes the patch
cleaner to review.

CatalogDAO
----------

* Still no javadocs

* needsIdRemapping() ? What is the purpose of this method. Seems to
expose implementation details uncessarily. Some explanation is required.

CatalogImpl
-----------

* add(WorkspaceInfo):

- Yes validate already contains this check, the check can probably be
removed

* setDefaultWorkspace(WorkspaceInfo)

- Remove the check, the contract of setDefaultWorkspace states that
the ws does not need to be previously added to the catalog

* postSave(CatatlogInfo)

- Why is the call to proxy.commit() commented out?

* CatalogSync

- Why a separate class for this? Why not just keep it as
Catalog.sync() and just call through to the dao.

MemoryCatalogDAO
----------------

* The save() methods seems to duplicate each other. The old catalog
factored this out into a common method. Again a pure refactor would have
maintained this I would think.

* getLayersByRsource():

- Some checks seems to be commented out? Were these checks added by
you, I actually could not find them in the original CatalogImpl class.

-Justin

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

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

Justin_Deoliveira · February 25, 2010, 3:01pm

Logging guard is a good habit and also a requirement I have put
forward for our (GeoSolutions) code.
There are plenty of measurements around the web that shows why we
should use it, especially with logging levels like FINE, FINER, etc..
Bottom line is, I would find it hard to ask ETj to not use this, since
it is usually easy to forget good habits :-).

Well this this case I would be astonished if there was a measurable different. And last time I checked GeoServer coding conventions trump ones used by any specific company. That said there is no hard and fast rule about logging guards and it can never hurt so no issue there. Sorry for being too picky.

There is however a hard and fast rule about curly braces though. And for good reason. As soon as people write code without curly braces on if statements and forget to set the proper formatting setup in their ide code essentially becomes unreadable.

Ciao,
Simone.

* I see a lot of commented out code. Just remove it, it makes the patch
cleaner to review.

CatalogDAO
----------

* Still no javadocs

* needsIdRemapping() ? What is the purpose of this method. Seems to
expose implementation details uncessarily. Some explanation is required.

CatalogImpl
-----------

* add(WorkspaceInfo):

   - Yes validate already contains this check, the check can probably be
removed

* setDefaultWorkspace(WorkspaceInfo)

   - Remove the check, the contract of setDefaultWorkspace states that
the ws does not need to be previously added to the catalog

* postSave(CatatlogInfo)

   - Why is the call to proxy.commit() commented out?

* CatalogSync

   - Why a separate class for this? Why not just keep it as
Catalog.sync() and just call through to the dao.

MemoryCatalogDAO
----------------

* The save() methods seems to duplicate each other. The old catalog
factored this out into a common method. Again a pure refactor would have
maintained this I would think.

* getLayersByRsource():

   - Some checks seems to be commented out? Were these checks added by
you, I actually could not find them in the original CatalogImpl class.

-Justin

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

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Geoserver-devel mailing list
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.

Simone_Giannecchini3 · February 25, 2010, 3:21pm

On Thu, Feb 25, 2010 at 4:01 PM, Justin Deoliveira <jdeolive@anonymised.com> wrote:

Logging guard is a good habit and also a requirement I have put
forward for our (GeoSolutions) code.
There are plenty of measurements around the web that shows why we
should use it, especially with logging levels like FINE, FINER, etc..
Bottom line is, I would find it hard to ask ETj to not use this, since
it is usually easy to forget good habits :-).

Well this this case I would be astonished if there was a measurable
different. And last time I checked GeoServer coding conventions trump
ones used by any specific company. That said there is no hard and fast
rule about logging guards and it can never hurt so no issue there. Sorry
for being too picky.

Well, I can give you the bkg for this rule.

For one of our client we have been using a custom logging lib that was
loggingo onto a JMS queue.
We were not using guarded logging and we were having some large
performance differences depending on the conditions of the system.
We investigated and we noticed that there was code inside the logging
lib that was building the JMS machinery every time before checking the
level.
So better guard the code than change the lib.

Another exafrom a couple of years ago. In another custom library (that
was C++), I noticed that logging seemed to be heavy.
I investigated and again, the library was building the final message
prior to checking the level which meant getting the local time and
formatting it and that can be expensive.
Again better guard the code than change the lib.

Another example if when in your message you get the stacktrace and
then you spit it out. This can be really heavy so better guarding the
code rather than gathering the stack trace and throwing it away.

In the end, I usually recommed/require to use it.

There is however a hard and fast rule about curly braces though. And for
good reason. As soon as people write code without curly braces on if
statements and forget to set the proper formatting setup in their ide
code essentially becomes unreadable.

No objections, you have all rights to stand out and enforce the rule.

Hope that helps a bit.

Ciao,
Simone.

Ciao,
Simone.

* I see a lot of commented out code. Just remove it, it makes the patch
cleaner to review.

CatalogDAO
----------

* Still no javadocs

* needsIdRemapping() ? What is the purpose of this method. Seems to
expose implementation details uncessarily. Some explanation is required.

CatalogImpl
-----------

* add(WorkspaceInfo):

- Yes validate already contains this check, the check can probably be
removed

* setDefaultWorkspace(WorkspaceInfo)

- Remove the check, the contract of setDefaultWorkspace states that
the ws does not need to be previously added to the catalog

* postSave(CatatlogInfo)

- Why is the call to proxy.commit() commented out?

* CatalogSync

- Why a separate class for this? Why not just keep it as
Catalog.sync() and just call through to the dao.

MemoryCatalogDAO
----------------

* The save() methods seems to duplicate each other. The old catalog
factored this out into a common method. Again a pure refactor would have
maintained this I would think.

* getLayersByRsource():

- Some checks seems to be commented out? Were these checks added by
you, I actually could not find them in the original CatalogImpl class.

-Justin

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

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Geoserver-devel mailing list
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.

------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Geoserver-devel mailing list
Geoserver-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/geoserver-devel

Andrea_Aime3 · February 25, 2010, 3:28pm

Simone Giannecchini wrote:

Well, I can give you the bkg for this rule.

For one of our client we have been using a custom logging lib that was
loggingo onto a JMS queue.
We were not using guarded logging and we were having some large
performance differences depending on the conditions of the system.
We investigated and we noticed that there was code inside the logging
lib that was building the JMS machinery every time before checking the
level.
So better guard the code than change the lib.

Another exafrom a couple of years ago. In another custom library (that
was C++), I noticed that logging seemed to be heavy.
I investigated and again, the library was building the final message
prior to checking the level which meant getting the local time and
formatting it and that can be expensive.
Again better guard the code than change the lib.

Another example if when in your message you get the stacktrace and
then you spit it out. This can be really heavy so better guarding the
code rather than gathering the stack trace and throwing it away.

In the end, I usually recommed/require to use it.

I don't really like to see the code riddled with ifs everywhere,
but I can also add another good reason to add guards: log4j logging calls are synchronized, they kill scalability if they're put in tight loops.

However, for the JMS case remember we're using a indirection level
to get to the real logging, that can be used to avoid the actual
logging calls so there is not really a need to guard INFO and above,
just the FINE ones in tight loops should be enough.

Cheers
Andrea

Justin_Deoliveira · February 25, 2010, 8:53pm

Very good reasons, thank you. I am now a converter and will guard all log statements

-Justin

On 2/25/10 10:28 AM, Andrea Aime wrote:

Simone Giannecchini wrote:

Well, I can give you the bkg for this rule.

For one of our client we have been using a custom logging lib that was
loggingo onto a JMS queue.
We were not using guarded logging and we were having some large
performance differences depending on the conditions of the system.
We investigated and we noticed that there was code inside the logging
lib that was building the JMS machinery every time before checking the
level.
So better guard the code than change the lib.

Another exafrom a couple of years ago. In another custom library (that
was C++), I noticed that logging seemed to be heavy.
I investigated and again, the library was building the final message
prior to checking the level which meant getting the local time and
formatting it and that can be expensive.
Again better guard the code than change the lib.

Another example if when in your message you get the stacktrace and
then you spit it out. This can be really heavy so better guarding the
code rather than gathering the stack trace and throwing it away.

In the end, I usually recommed/require to use it.

I don't really like to see the code riddled with ifs everywhere,
but I can also add another good reason to add guards: log4j logging
calls are synchronized, they kill scalability if they're put in tight
loops.

However, for the JMS case remember we're using a indirection level
to get to the real logging, that can be used to avoid the actual
logging calls so there is not really a need to guard INFO and above,
just the FINE ones in tight loops should be enough.

Cheers
Andrea

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