Re: [RT] New Feature Flags API / Impl
On Fri, Jan 3, 2014 at 3:50 PM, Carsten Ziegeler cziege...@apache.org wrote: ...I'm wondering if we still need the FeatureProvider? A Feature could be an OSGi service by itself I agree, Feature then just needs a boolean isEnabled(ProviderContext context) method. The downside is that you then need one Feature service per feature, but a component can also register multiple Feature services if needed to make configurations/setup simpler. -Bertrand
Re: [RT] New Feature Flags API / Impl
I've moved the implementation to trunk. I'm wondering if we still need the FeatureProvider? A Feature could be an OSGi service by itself. Carsten 2013/12/31 Carsten Ziegeler cziege...@apache.org I've updated the implementation as discussed, however I'm not that happy with the new service interface names. I came up with ResourceHiding and ResourceTypeMapper...I hope someone can come up with better names :) Carsten 2013/12/31 Carsten Ziegeler cziege...@apache.org Yes, sure - name and description are good Carsten 2013/12/31 Bertrand Delacretaz bdelacre...@apache.org Hi Carsten, On Tue, Dec 31, 2013 at 11:46 AM, Carsten Ziegeler cziege...@apache.org wrote: ...I thought about making this more generic and have an adaptTo method on the Feature class instead and each functionality (resource type mapping, hiding resources) gets an own interface and either the feature can be adapted to this or now. So calling adaptTo would be the equivalent of hasXXX or canXXX... Works for me, good idea! I'd still keep the getDescription() method on the Feature, I think this it's useful even if we don't use it internally. -Bertrand -- Carsten Ziegeler cziege...@apache.org -- Carsten Ziegeler cziege...@apache.org -- Carsten Ziegeler cziege...@apache.org
Re: [RT] New Feature Flags API / Impl
Hi Carsten, On Mon, Dec 30, 2013 at 10:11 AM, Carsten Ziegeler cziege...@apache.org wrote: ... 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... Ok, works for me. I was asking because we sometimes use .api and sometimes not. 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.. o.a.s.featureflags works for me. ...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)... Agreed, a warning log is good enough. I'll reply to the hideResource thing separately is it's more complex. -Bertrand
Re: [RT] New Feature Flags API / Impl
Hi Carsten,
On Mon, Dec 30, 2013 at 10:11 AM, Carsten Ziegeler cziege...@apache.org wrote:
Bertrand wrote:
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...
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...
So IIUC the when hiding a resource or changing its resource type the
scenario is as follows?
1. ResourceResolver checks all existing features to see which ones are
active in the given context.
2. For each active feature, ResourceResolver calls
FeatureProvider.hideResource and
FeatureProvider.getResourceTypeMapping to find out if resource
manipulations are needed.
If I'm correct, that's a bit expensive for our 10'000 features (yes I
bet creative users will get there ;-)
How about making a Feature an object, instead of just a String:
Feature {
String getName();
String getDescription();
boolean hasResourceTypeMapping();
boolean canHideResource();
}
The ResourceResolver can then consider only the features which are
marked for resource type mapping or for hiding resources. And having a
feature description can be useful for troubleshooting, webconsole etc.
-Bertrand
Re: [RT] New Feature Flags API / Impl
Hi Bertrand,
2013/12/31 Bertrand Delacretaz bdelacre...@apache.org
So IIUC the when hiding a resource or changing its resource type the
scenario is as follows?
1. ResourceResolver checks all existing features to see which ones are
active in the given context.
This is done outside of the resource resolver once per request - once the
resource
resolver is invoked, the feature flags implementation knows already which
features are
active.
2. For each active feature, ResourceResolver calls
FeatureProvider.hideResource and
FeatureProvider.getResourceTypeMapping to find out if resource
manipulations are needed.
It's not done by the resource resolver, but basically yes.
If I'm correct, that's a bit expensive for our 10'000 features (yes I
bet creative users will get there ;-)
Well, I personally doubt that this is realistic and will create all kinds
of problems if used in such an extensive number, but I guess it's pointless
to argue about a fictional number anyway :) So, I agree we should prepare
for the worse...
How about making a Feature an object, instead of just a String:
Feature {
String getName();
String getDescription();
boolean hasResourceTypeMapping();
boolean canHideResource();
}
The ResourceResolver can then consider only the features which are
marked for resource type mapping or for hiding resources. And having a
feature description can be useful for troubleshooting, webconsole etc.
Yes, I was thinking about this as well - especially as we might have new
functionality to be influenced by features in the future (like service
lookup etc.), so I thought about making this more generic and have an
adaptTo method on the Feature class instead and each functionality
(resource type mapping, hiding resources) gets an own interface and either
the feature can be adapted to this or now. So calling adaptTo would be the
equivalent of hasXXX or canXXX.
WDYT?
Regards
Carsten
--
Carsten Ziegeler
cziege...@apache.org
Re: [RT] New Feature Flags API / Impl
Hi Carsten, On Tue, Dec 31, 2013 at 11:46 AM, Carsten Ziegeler cziege...@apache.org wrote: ...I thought about making this more generic and have an adaptTo method on the Feature class instead and each functionality (resource type mapping, hiding resources) gets an own interface and either the feature can be adapted to this or now. So calling adaptTo would be the equivalent of hasXXX or canXXX... Works for me, good idea! I'd still keep the getDescription() method on the Feature, I think this it's useful even if we don't use it internally. -Bertrand
Re: [RT] New Feature Flags API / Impl
Yes, sure - name and description are good Carsten 2013/12/31 Bertrand Delacretaz bdelacre...@apache.org Hi Carsten, On Tue, Dec 31, 2013 at 11:46 AM, Carsten Ziegeler cziege...@apache.org wrote: ...I thought about making this more generic and have an adaptTo method on the Feature class instead and each functionality (resource type mapping, hiding resources) gets an own interface and either the feature can be adapted to this or now. So calling adaptTo would be the equivalent of hasXXX or canXXX... Works for me, good idea! I'd still keep the getDescription() method on the Feature, I think this it's useful even if we don't use it internally. -Bertrand -- Carsten Ziegeler cziege...@apache.org
Re: [RT] New Feature Flags API / Impl
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 IteratorString 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
Re: [RT] New Feature Flags API / Impl
Hi Carsten, On Thu, Dec 19, 2013 at 6:02 AM, Carsten Ziegeler cziege...@apache.org wrote: ...Everything can be found at: http://svn.apache.org/repos/asf/sling/whiteboard/feature-flags/ ... Today is Big Reviews Day, finally found time to look at that, thanks for it! It looks good to me for our current needs, here are my comments: 1) Why not put the API in the org.apache.sling.extensions.featureflags.api package? It's at org.apache.sling.extensions.featureflags currently. 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. 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 IteratorString for lists of feature names? 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. -Bertrand (*) Except for troubleshooting, but that's covered by RequestProgressTracker which is accessible via ProviderContext.getRequest()
Re: [RT] New Feature Flags API / Impl
If no one complains I'll move this stuff into the contrib section - we can
change it there anyway
Thanks
Carsten
2013/12/19 Carsten Ziegeler cziege...@apache.org
Hi Amit
thanks for the feedback - yes, good point with active vs available. I'll
change that.
Yes, as the implementation manages the contexts, it first calls isEnabled
on the providers to know which features are enabled and then only calls the
other methos like hideResource if the feature is really enabled. This
avoids double checking. I'll make some updates to the javadocs
Thanks
Carsten
2013/12/19 Amit.. Gupta. amitg...@adobe.com
Hi Carsten,
In FeatureManager
String[] getFeatureNames();
Javadoc says it returns all active features. Active might give an
impression of this only returning enabled features, but rather it returns
all available features. Changing the javadoc will also align with
FeatureManager.isAvailable.
And, I could not understand how context is passed on in
FeatureManager.hideResource(final String featureName, final Resource
resource)
Is it the caller's responsibility to first check isEnabled(featureName,
context), before calling hideResource?
Thanks,
-Amit
-Original Message-
From: Carsten Ziegeler [mailto:cziege...@apache.org]
Sent: 19 December 2013 10:33
To: dev@sling.apache.org
Subject: [RT] New Feature Flags API / Impl
Hi,
I've started with a new approach to implement the feature flags - the
focus is on the API, which means what features do our feature flags have
and how can they report it. (The implementation should be functional but I
haven't checked it yet).
I went back and forth with different approaches based on Bertrands
prototype and all the discussions and I think this approach is the most
promising one:
The central service for client code is the Features service:
public interface Features {
String[] getFeatureNames();
boolean isAvailable(String featureName);
ClientContext createClientContext(ResourceResolver resolver);
ClientContext createClientContext(SlingHttpServletRequest request);
ClientContext getCurrentClientContext();
}
This can be used to find out which features are available and to create a
client context - the context is either based on a request or on a resource
resolver.
The client context in turn can be used to find out whether a feature is
enabled:
public interface ClientContext {
boolean isEnabled(String featureName);
CollectionString getEnabledFeatures(); }
So a script can use this, to do things differently etc.
In addition the Features services provides the current client context
which is automatically available for a request.
Now, somehow we need an API to define which feature is available and what
it does - I came up with an extended version of the FeatureProvider (and
I'm not so sure about this one):
public interface FeatureProvider {
String [] getFeatureNames();
boolean isEnabled(String featureName, ProviderContext context);
MapString, String getResourceTypeMapping(String featureName);
boolean hideResource(String featureName, Resource resource); } It's
similar to the Bertrand's version, except that it takes a ProviderContext
(which allows access to the request and resource resolver) and has two
additional methods control the two most wanted features: hiding resources
and changing of resource types.
We could then implement this provider reading OSGi configurations or
whatever to define features.
All constructive feedback welcome - and as I said, let's first define the
API
Everything can be found at:
http://svn.apache.org/repos/asf/sling/whiteboard/feature-flags/
Regards
Carsten
--
Carsten Ziegeler
cziege...@apache.org
--
Carsten Ziegeler
cziege...@apache.org
--
Carsten Ziegeler
cziege...@apache.org
Re: [RT] New Feature Flags API / Impl
On Tue, Dec 24, 2013 at 11:12 AM, Carsten Ziegeler cziege...@apache.org wrote: ... If no one complains I'll move this stuff into the contrib section - we can change it there anyway... Fine with me, I haven't found time to review yet but evolving it in there is not a problem IMO, thanks! -Bertrand
RE: [RT] New Feature Flags API / Impl
... If no one complains I'll move this stuff into the contrib section - +1 -Amit -Original Message- From: Bertrand Delacretaz [mailto:bdelacre...@apache.org] Sent: 24 December 2013 16:03 To: dev Subject: Re: [RT] New Feature Flags API / Impl On Tue, Dec 24, 2013 at 11:12 AM, Carsten Ziegeler cziege...@apache.org wrote: ... If no one complains I'll move this stuff into the contrib section - we can change it there anyway... Fine with me, I haven't found time to review yet but evolving it in there is not a problem IMO, thanks! -Bertrand
Re: [RT] New Feature Flags API / Impl
Hi Amit
thanks for the feedback - yes, good point with active vs available. I'll
change that.
Yes, as the implementation manages the contexts, it first calls isEnabled
on the providers to know which features are enabled and then only calls the
other methos like hideResource if the feature is really enabled. This
avoids double checking. I'll make some updates to the javadocs
Thanks
Carsten
2013/12/19 Amit.. Gupta. amitg...@adobe.com
Hi Carsten,
In FeatureManager
String[] getFeatureNames();
Javadoc says it returns all active features. Active might give an
impression of this only returning enabled features, but rather it returns
all available features. Changing the javadoc will also align with
FeatureManager.isAvailable.
And, I could not understand how context is passed on in
FeatureManager.hideResource(final String featureName, final Resource
resource)
Is it the caller's responsibility to first check isEnabled(featureName,
context), before calling hideResource?
Thanks,
-Amit
-Original Message-
From: Carsten Ziegeler [mailto:cziege...@apache.org]
Sent: 19 December 2013 10:33
To: dev@sling.apache.org
Subject: [RT] New Feature Flags API / Impl
Hi,
I've started with a new approach to implement the feature flags - the
focus is on the API, which means what features do our feature flags have
and how can they report it. (The implementation should be functional but I
haven't checked it yet).
I went back and forth with different approaches based on Bertrands
prototype and all the discussions and I think this approach is the most
promising one:
The central service for client code is the Features service:
public interface Features {
String[] getFeatureNames();
boolean isAvailable(String featureName);
ClientContext createClientContext(ResourceResolver resolver);
ClientContext createClientContext(SlingHttpServletRequest request);
ClientContext getCurrentClientContext();
}
This can be used to find out which features are available and to create a
client context - the context is either based on a request or on a resource
resolver.
The client context in turn can be used to find out whether a feature is
enabled:
public interface ClientContext {
boolean isEnabled(String featureName);
CollectionString getEnabledFeatures(); }
So a script can use this, to do things differently etc.
In addition the Features services provides the current client context
which is automatically available for a request.
Now, somehow we need an API to define which feature is available and what
it does - I came up with an extended version of the FeatureProvider (and
I'm not so sure about this one):
public interface FeatureProvider {
String [] getFeatureNames();
boolean isEnabled(String featureName, ProviderContext context);
MapString, String getResourceTypeMapping(String featureName);
boolean hideResource(String featureName, Resource resource); } It's
similar to the Bertrand's version, except that it takes a ProviderContext
(which allows access to the request and resource resolver) and has two
additional methods control the two most wanted features: hiding resources
and changing of resource types.
We could then implement this provider reading OSGi configurations or
whatever to define features.
All constructive feedback welcome - and as I said, let's first define the
API
Everything can be found at:
http://svn.apache.org/repos/asf/sling/whiteboard/feature-flags/
Regards
Carsten
--
Carsten Ziegeler
cziege...@apache.org
--
Carsten Ziegeler
cziege...@apache.org
RE: [RT] New Feature Flags API / Impl
Hi Carsten,
In FeatureManager
String[] getFeatureNames();
Javadoc says it returns all active features. Active might give an impression of
this only returning enabled features, but rather it returns all available
features. Changing the javadoc will also align with FeatureManager.isAvailable.
And, I could not understand how context is passed on in
FeatureManager.hideResource(final String featureName, final Resource resource)
Is it the caller's responsibility to first check isEnabled(featureName,
context), before calling hideResource?
Thanks,
-Amit
-Original Message-
From: Carsten Ziegeler [mailto:cziege...@apache.org]
Sent: 19 December 2013 10:33
To: dev@sling.apache.org
Subject: [RT] New Feature Flags API / Impl
Hi,
I've started with a new approach to implement the feature flags - the focus is
on the API, which means what features do our feature flags have and how can
they report it. (The implementation should be functional but I haven't checked
it yet).
I went back and forth with different approaches based on Bertrands prototype
and all the discussions and I think this approach is the most promising one:
The central service for client code is the Features service:
public interface Features {
String[] getFeatureNames();
boolean isAvailable(String featureName);
ClientContext createClientContext(ResourceResolver resolver);
ClientContext createClientContext(SlingHttpServletRequest request);
ClientContext getCurrentClientContext();
}
This can be used to find out which features are available and to create a
client context - the context is either based on a request or on a resource
resolver.
The client context in turn can be used to find out whether a feature is
enabled:
public interface ClientContext {
boolean isEnabled(String featureName);
CollectionString getEnabledFeatures(); }
So a script can use this, to do things differently etc.
In addition the Features services provides the current client context
which is automatically available for a request.
Now, somehow we need an API to define which feature is available and what it
does - I came up with an extended version of the FeatureProvider (and I'm not
so sure about this one):
public interface FeatureProvider {
String [] getFeatureNames();
boolean isEnabled(String featureName, ProviderContext context);
MapString, String getResourceTypeMapping(String featureName);
boolean hideResource(String featureName, Resource resource); } It's similar
to the Bertrand's version, except that it takes a ProviderContext (which allows
access to the request and resource resolver) and has two additional methods
control the two most wanted features: hiding resources and changing of resource
types.
We could then implement this provider reading OSGi configurations or whatever
to define features.
All constructive feedback welcome - and as I said, let's first define the API
Everything can be found at:
http://svn.apache.org/repos/asf/sling/whiteboard/feature-flags/
Regards
Carsten
--
Carsten Ziegeler
cziege...@apache.org
