[mapguide-internals] RE: Initial review on RFC90 code

[Zac Spitzer]

Uv just moved back to Berlin and a new job...

ie help wanted? :) anyone want to help?

your review notes where fine me thinks, just the age old
question of time available....


On 3 May 2010 22:47, Trevor Wekel <trevor_wekel at otxsystems.com> wrote:
> Hmm... I wonder if my tone was a little strong for the review.  If it was, I apologize.
>
> UV, if you have time, it would be great if you could provide me with some feedback on the review.  The PSC was hoping to get RFC 90 into MapGuide 2.2 but the beta is already a month late.
>
> Regards,
> Trevor
>
>
> -----Original Message-----
> From: Trevor Wekel
> Sent: April 27, 2010 12:10 PM
> To: 'MapGuide Internals Mail List'
> Subject: Initial review on RFC90 code
>
> Hi UV,
>
> Here are some comments from my initial review of the RFC 90 code.  I will do a follow up review once the RFC has been updated and code changes have been made.
>
> Thank you,
> Trevor
>
>
> General Comments
> ----------------
> ----------------
>
> RFC 90 documentation in Trac is incomplete.  All serverconfig.ini parameters and new function signatures should be documented.  A general algorithm overview would also be good.
>
> Based on my initial review, the following items should be documented:
> - useMetaTiles and LockMethod serverconfig.ini parameters.
> - Behavioural modification to AGGRenderer::Save and AGGImageIO.cpp
> - New MgTileCache behaviour (see below, I believe some refactoring would be appropriate)
>
> File specific comments
> ----------------------
> ----------------------
>
> RenderingService.h
> ProxyRenderingService.h
> -----------------------
> - new RenderFromMetaTile signature commented out, should it be removed?
>
> TileService.h
> -------------
> - space change is not necessary.
>
> ConfigProperties.cpp
> ConfigProperties.h
> ------------------
> - TileServicePropertyUseMetaTiles, TileServicePropertyLockMethod should be doc'd in RFC.
> - New properties should be added to MgConfigProperties::sm_cviTileServiceProperties array
>
>
> AGGRenderer.cpp
> ---------------
> - "META" image format could be defined in MgImageFormats as an INTERNAL_API
> - new ::SaveMethod uses format = "META".  I am not 100% clear on how this all fits together.  Some algorithm documetation in the RFC would be helpful.
> - Can assert be replaced by a throw (MgInvalidArgumentException)?
>
> AggImageIO.cpp
> --------------
> - Please remove commented out code.
>
> Server.sln
> ----------
> - Why were "Static" configurations added?
>
> ServerRenderingService.vcproj
> -----------------------------
> - Rendering Service should not depend on Tile service headers.
>
> ServerRenderingService.cpp
> --------------------------
> - GetTileCoords()
> What purpose does tileColOffset serve?
> Could this method be moved into MgMap as an INTERNAL_API method which takes tileWidth and tileHeight as parameters?
>
> - RenderTileFromMetaTile()
> Can asserts be replaced by MgInvalidArgumentException, MgOutOfRangeException?
> Should bsimp->GetLength() be tested for 0 < size < 2^31 before casting?
> Why is MyByteSource required?  The direct cast is fragile.
>
> ServerTileService.cpp
> ---------------------
> - RFC documentation for sm_creationCutoffTime, sm_pollingInterval, sm_useMetaTiles, sm_lockMethod
>
> - sm_creationCutoffTime and sm_pollingInterval should use same time base.
>
> - MgServerTileService::MgServerTileService() : MgTileService()
> METAMAXDIM should be moved to configuration parameters MG_CONFIG_MAX_METATILE_DIM
> Error message should use MgConfigurationPropertyValueIsOutOfRange
> <sstream> use could be dropped.
>
> - DetectLockFile()
> ------------------
> This functionality should be moved into MgTileCache and probably renamed to WaitForTileLock.
> Is a subsecond sleep really necessary?
> ACE_OS::sleep (const ACE_Time_Value &tv) may simplify code
>
> - GetTile()
> -----------
> Since there is no signature change to the public API, APIVERSION 1.2 is not really required.
>
> - GetSingleTile()
> -----------------
> MgFoundBlockingLockFile must be added to resource string file(s) in MapGuideCommon/Resources
>
> - GetMetaTile()
> ---------------
> Locking methods should be documented in the RFC.
> Algorithm can be simplified by only generating a lock for the left top corner.  Meta tiling will be always on or always off.  For future work, each map will also be meta tiled or not.
> I believe there is too much special case locking logic coded here.  The locking logic should be moved into MgTileCache (LockTile(), UnlockTile(), WaitForTileLock()).
> All error messages must be defined in MapGuideCommon/Resources
> <sstream> use could be dropped.
>
> TileCache.h
> -----------
> - GetLockMap()
> Lock map structure should not be directly exposed using GetLockMap().  LockTile() and UnlockTile() methods should be added.  New methods should hide sm_lockMethod details from the rest of the code.
>
> serverconfig.ini
> ----------------
> - Font names seem to have changed.  Please check editor.
>
>
> _______________________________________________
> mapguide-internals mailing list
> mapguide-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/mapguide-internals
>
>



-- 
Zac Spitzer
Solution Architect / Director
Ennoble Consultancy Australia
http://www.ennoble.com.au
http://zacster.blogspot.com
+61 405 847 168