On Wed, Nov 17, 2010 at 12:02 PM, Angelo van der Sijpt <angelo.vandersijpt at luminis.eu> wrote: > Hi, > > Good to see we have a file-based storage for our tenants now! > > Still, there are some things I don't really understand. > - I noticed the storage provider can be configured using a system property; > why not use the ConfigurationAdmin for this?
Good point. I was writing this under the assumption I might have to stick it under ConfigAdmin itself, but I see the Felix ConfigiAdmin has its own filebased storage allready (and uses a Java Property to allow overriding bundle storage). I'll change this. > - The FSTSP synchronizes all its methods; this it not necessary, as the > TenantStorageProvider javadoc states 'The TenantStorageProvider can assume > that synchronization will be handled externally.' I see... but I dont feel comfortable not synchronizing when I know my impl of a published service cannot handle it. On the otherhand this is an SPI so maybe I'm being a little too defensive :) > - It seems you use finally a little differently than I do (e.g., in > FSTentantIdList, the m_file is final, but the m_tenantIdList is not). Our > coding guideline does not state anything about this; should we include that? > Something like "members should be made final if they can, and local variables > should only be made final when they have to be". Seems I forgot to put the final modifier on m_tenantIdList. In general I like making things final where possible (and sometimes relevant). No strong opinion with regard to local variables though so +1 for your proposal :) Regards, Bram
