Hi Bertrand, thanks for the feedback, answers inline:
2013/12/27 Bertrand Delacretaz <bdelacre...@apache.org> > > 1) Why not put the API in the > org.apache.sling.extensions.featureflags.api package? It's at > org.apache.sling.extensions.featureflags currently. > > I don't have a strong preferences, but I always feel that "api" in the package name is superfluous - its the only visible package anyway. Looking at the package again, we should definitely remove "extensions" from the package name, so either o.a.s.featureflags or o.a.s.api.featureflags > 2) How do you envision handling feature naming collisions between > FeatureProviders? We might namespace feature names by requiring all > values returned by the getFeatureNames() method of a given > FeatureProvider to start with a common prefix followed by a dot, and > the prefix must be unique system-wide. > > If there is more than one provider for the same feature name, the one with the higher service ranking wins and the situation should be logged (I think, it is not right now) While namespacing helps, it still can lead to collisions - if two providers use the same namespace you end up in the same situation. So I think, detecting and logging the situation, have a clear resolution of the situation (who wins) and adding guidelines on how to define feature names (like using a namespace prefix like syntax) should do the trick > 3) Considering Roy's remark that we'll get about 1024 features in a > Sling instance, and user's creativity which will probably boost that > to 10'000, shall we use Iterator<String> for lists of feature names? > > We hold all feature names in memory anyway, so an Iterator does not really help and usually an Iterator makes an harder to use API as you can't reuse the iterator. > 4) I'm not sure about FeatureProvider.hideResource(featureName, > Resource) - as seen from the ResourceResolver, IMO you just care > whether a given FeatureProvider wants to hide a Resource or not, you > don't really care (*) which feature name causes the Resource to be > hidden. So maybe just hideResource(Resource, ProviderContext) ? The > same might apply to FeatureProvider.getResourceTypeMapping which would > then just take a ProviderContext - but maybe I'm missing something. > > A provider might provide more than a single feature, so it needs to know which feature definition to use for the two mentioned methods. The provider can be sure that the caller already checked that the feature is active, that's why the ProviderContext is not needed anymore. The provider does not need to do any checks anymore and can simply hide the resource. This optimizes the operations flow a little bit, otherwise the provider would need to check on each and every call if one of its features is enabled. And in addition, as we might have overlapping feature names (see above) a particalur feature from this provider might not be active Regards Carsten > -Bertrand > > (*) Except for troubleshooting, but that's covered by > RequestProgressTracker which is accessible via > ProviderContext.getRequest() > -- Carsten Ziegeler cziege...@apache.org
