Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance the bells-1.0 feature to support the configuration of BELL service properties #17414

Closed
dazavala opened this issue Jun 3, 2021 · 4 comments · Fixed by #21708
Closed
Assignees
Labels

Comments

@dazavala
Copy link
Contributor

dazavala commented Jun 3, 2021

For epic #19368

Consumers currently use Liberty properties to configure services provided by their BELL libraries. We could augment the <bell> metatype to allow the declaration of properties for services named within a <bell> configuration, and enhance the feature runtime to set these properties on service implementation classes during BELL initialization. Here's a sample BELL configuration.

    <server>
          <featureManager>
              <feature>servlet-3.1</feature>
              <feature>bells-1.0</feature>
       </featureManager>
       <library id="exampleBellLib" name="exampleBellLib" description="exampleBellLib">
           <fileset dir="${server.output.dir}/sharedLib" includes="exampleBellLib.jar" />
       </library>
       <bell libraryRef="exampleBellLib"/>

A few concerns come to mind:

  1. We could exploit this: The current bells-1.0 feature support collects properties in META-INF/services files; it also passes them into BundleContext.registerService() and caches them in the corresponding service factories. This configuration must be proprietary, because neither the java provider-configuration nor the service loader docs mention property support.
  1. This enhancement will require a proprietary method of setting properties on a service class. Do we care? The runtime would collect service properties from the <bell> configuration into the service factory. Before returning the service instance the factory would invoke instance methods setX(_xValue_) -- a la JavaBeans -- for all properties, or invoke instance method init(map) with a Map of properties.

  2. Bell documentation says the user may declare the name of the Meta-inf service provided by the BELL library -- i.e., a single service name. If this is a valid logical restriction, then the configuration of service properties could simply entail adding a property attribute to the bell OCD, as shown below. Note that the runtime does not enforce this restriction. So, if we expect the user to name more than one service in a <bell>, then the Bells metatypes will require another OCD for the server, with a required name attribute and an optional attribute for service properties. This sounds like a breaking change to me, which is circumvented by 1).

    <OCD name="%bell" description="%bell.desc"
             id="com.ibm.ws.classloading.bell"
             ibm:alias="bell">
        <AD name="%bell.library.ref" description="%bell.library.ref.desc"
                id="libraryRef", required="true", type="String", ibm:type="pid"
                ibm:reference="com.ibm.ws.classloading.sharedlibrary"
         />
        <AD id="library.target" type="String" required="true" default="(service.pid=${libraryRef})" name="internal" description="internal use only" ibm:final="true" />
        <AD id="service" name="%service.name" description="%service.name.desc" required="false" type="String" cardinality="2147483647"/>
        **<AD id="service.property" name="%service.property.name" description="%service.property.desc" required="false" type="String" cardinality="2147483647"/>**
    </OCD>
@dazavala dazavala self-assigned this Jun 3, 2021
@dazavala
Copy link
Contributor Author

dazavala commented Jun 3, 2021

FYI @NottyCode, @tjwatson

@dazavala dazavala changed the title DRAFT: Enhance the bells-1.0 feature to support the configuration of BELL service properties Enhance the bells-1.0 feature to support the configuration of BELL service properties Jun 3, 2021
@dazavala dazavala added the In Progress Items that are in active development. label Sep 16, 2021
@dazavala
Copy link
Contributor Author

dazavala commented May 24, 2022

Considerations from Friday's UFO presentation

Errata: TomW is correct. bells-1.0 will launch named services XOR services that are found in the referenced library. Not both as I had claimed.

Service property configuration

image

  1. The name <serviceProperties/> implies properties are passed to OSGi service registration in addition to service impls. Currently bells-1.0 collects service properties from META-INF/services files, passes them into BundleContext.registerService() and caches them in the corresponding service factories. We don't know why the current behavior exists -- can we replace the existing service property support with this enhancement? Or should we instead use the name <properties/>? (tomW)
  2. Specifying service names in <serviceProperties/> introduces the need for another warning message indicating the user mistakenly named a service that does not exist. We could remove the service attribute since the proposed design maps properties to all named services XOR all found services. Or perhaps we promote the bells feature and introduce a new configuration metatype that affords service-specific configuration going forward, something like <serviceConfig/>, and tag the existing service attribute as internal (nateR). I also intended to propose the idea:

image

Service property propagation
The design to pass properties to service impl instances through a ctor having one argument of type Map<string,string> remains as proposed. There seems no benefit to introduce a BELL SPI/annotation, to support property value types other than string, nor to pass properties through javabean setters. (several)

@dazavala
Copy link
Contributor Author

dazavala commented May 24, 2022

Resolutions per today's design issues meeting

  1. Change the child metatype name from <serviceProperties/> to <properties/> to clarify the intent of the property configuration. Support for service properties declared in META-INF/service metadata was introduced per RFE and cannot be changed. Also, replacing this support would require a policy for exposing PI. Since these properties cannot be made available via the OSGi service registry, and the properties will now logically scope to the BELL (see 2), the metatype name should be changed.
  2. Remove the service attribute from the <properties/> metatype and keep the proposed behavior to propagate properties to all named services XOR all found services. There's no benefit to passing properties into a specific service that is not already named in the BELL configuration. Plus, removing the service attribute implies the properties scope to the BELL. This obviates the need for service-specific configuration going forward, as future enhancements will also scope new configuration to the BELL. The user will simply declare more BELLs with configuration specific to services either named in the BELL or contained in the library referenced by the BELL. Lastly, the BELL <properties/> child element is necessary to separate end-user configuration from system configuration -- i.e. the user must not declare Liberty properties in the <bell/> (or any root metatype).
  3. Solutions for property propagation may involve constructor injection, should support update(), and should support the no-op constructor. Supporting the no-op constructor implies an update() method in the service implementation class. Update means the service is stopped+destroyed then recreated+started with updated configuration. Failure to support the no-op constructor will likely result in a RFE. More on this as we revise the propagation scheme.

@dazavala dazavala linked a pull request Jul 14, 2022 that will close this issue
@dazavala dazavala reopened this Jul 18, 2022
@dazavala dazavala added target:beta The Epic or Issue is targetted for the next beta target:200010-beta labels Aug 3, 2022
@dazavala dazavala added target:220011 and removed In Progress Items that are in active development. target:220011 labels Sep 12, 2022
@dazavala
Copy link
Contributor Author

Closing. This work delivered to 220010 beta. We can reopen this issue if additional work is needed for GA.

@cbridgha cbridgha moved this to Implemented in Design Issues Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Implemented
Development

Successfully merging a pull request may close this issue.

1 participant