Hi Marcel On Tue, Jul 7, 2020 at 12:02 PM Marcel Reutegger <[email protected]> wrote: > > Hi, > > Thanks for the feedback Julian. > > On 07.07.20, 10:45, "Julian Sedding" <[email protected]> wrote: > > I'm not sure about the aspect of the implementation, that FeatureToggle > > is Closeable and probably often short-lived. Given that the > > FeatureToggleAdapter is registered with the whiteboard, and thus likely > > with the OSGi service registry, this _may_ put unnecessary load on the > > service registry. > > If used as a short-lived object, that is indeed a problem. My intention > with the FeatureToggle is actually that it is long-lived, though it can > obviously also be used differently. The try-with-resource block in the > tests is just convenient.
It seems I misinterpreted the use of try-with-resource to indicate short-lived toggles. I don't think it's possible to enforce long-lived toggles, but it can certainly be encouraged in documentation. If it turns out that we get problems with short-lived toggles, they can still be solved later. I think your API would allow such changes in the future. > > > And lastly, even if a FeatureToggleAdapter is already registered for a > > feature, a new service would be registered if the same code was run in a > > second thread. > > This is by design. It is valid to have multiple feature toggles registered > with the same name. It's not the primary use case, but they can be used > that way. Ack. I assume they would get the same enabled/disabled state. > > > From an OSGi perspective, I would lean towards a long-lived singleton > > service that can be toggled. The FeatureToggle could then be adjusted to > > retrieve the matching service if available, or otherwise register its > > own. > > I'm not sure I understand. Can you elaborate what you have in mind? I meant that the implementation of Feature.newFeatureToggle() (maybe rename to newFeature after the class name changes?) could be adjusted from "always registering a FeatureToggle" to "returning an existing FeatureToggle service with the same name and register a new one only if none is available". Not sure this would work after you stated above "it is valid to have multiple feature toggles registered with the same name", even though I don't understand the benefit of registering multiple toggles. > > > Regarding the API, I would probably rename FeatureToggle to Feature and > > FeatureToggleAdapter to FeatureToggle. But that's of course a matter of > > taste. > > Thanks for the suggestion. I like it. :) > > > Also, I would add an "isEnabled" method to FeatureToggleAdapter, in > > order to allow the code setting the toggle to introspect the current > > state. > > I considered this as well, but did not see a use case for it. What would > you do with this method? I don't have a use case, but could imagine that introspection of the state could be useful for reporting (e.g. a web-console report of all active toggles and their state). I understand the desire to keep an API minimal, but on the other hand I find it frustrating when an API doesn't offer seemingly obvious features (obvious in my mind anyways). > > Regards > Marcel > Regards Julian
