[GitHub] [jackrabbit-filevault] stoerr commented on pull request #136: JCRVLT-517 fix package cache init in OSGi containers

[GitBox <gi...@apache.org>]

stoerr commented on pull request #136:
URL: https://github.com/apache/jackrabbit-filevault/pull/136#issuecomment-828974792


   I did consider getting rid of lazy loading completely when doing #135 , but I'm afraid the original author might have had a good reason to do it. The problem is that the method loadPackages() is a quite expensive method, as it accesses the filesystem and opens all files to collect metadata. If the filesystem is a network filesystem (which might even make sense for distributing packages across instances), then this can even block indefinitely. That's fine in tests (at least within Jackrabbit Vault the FSPackageRegistryIT and your new test seem to be the only places the additional constructors are used), but that's probably not OK during OSGI activation. That might block your bundles from coming up and in the worst case you couldn't even get into the console to switch it off / change the path. And the registry might even not be used at all later...
   
   So I'd strongly suggest to keep lazy loading for the activate method. You could consider encapsulating stateCache to switch it completely to lazy loading to be consistent (in the original code getInstallState / setInstallState methods also didn't call the loadPackages()), but that does change the behavior of the public constructors somewhat, so it might theoretically break e.g. a test someone did in a project using Vault, though that's admittedly a strange case. Not sure whether it's worth the risk.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org