Implementation of "160. Feature Launcher Service Specification" ( issues 2, 3, 6, 7, 17 )

[ideas-into-software]

• Implementation of 'ArtifactRepository' for remote repository type

• Providing Feature configuration to the system via Configuration Admin Service

• Selecting OSGi framework implementation

• Providing Framework Launch Properties

• Refactoring / fixes / improvements

[ideas-into-software]

#2
#3
#6
#7
#17

[ideas-into-software]

In addition to PR review comments you may have Tim, here's few items I wanted to mention:

• I added com.kentyou.featurelauncher.impl.repository.EnhancedArtifactRepository as org.osgi.service.featurelauncher.repository.ArtifactRepository only defines getArtifact(ID) method which returns InputStream, while having method which returns Path to artifact ( like getArtifactPath(ID) added to EnhancedArtifactRepository ) is very useful as well; shouldn't that be worked into the API ( org.osgi.service.featurelauncher.repository.ArtifactRepository ) or leave as is ( i.e. have EnhancedArtifactRepository which extends ArtifactRepository ) ?

• that additional getArtifactPath(ID) method made it much easier to utilize Java's built-in class loader ( URLClassLoader ), and avoid creating custom class loader which handles JARs from InputStream; is that acceptable or instead of URLClassLoader create custom class loader which handles JARs from InputStream ?

• I wasn't sure if logic for selecting framework ( com.kentyou.featurelauncher.impl.FrameworkFactoryLocator.selectFrameworkFactory(FeatureExtension, List<ArtifactRepository>) ) should be repackaged to specialized FeatureExtensionHandler, e.g. LaunchFrameworkFeatureExtensionHandlerImpl - but, the only method defined on FeatureExtensionHandler returns Feature.. and this select framework "Feature extension" does not require pre-processing, it is FrameworkFactory that must be returned; please clarify;

• since path to local Maven repository is also needed to create remote repository, as a ( temporary ? ) work around this problem, I had to use configuration properties passed org.osgi.service.featurelauncher.repository.ArtifactRepositoryFactory.createRepository(URI, Map<String, Object>) to provide this path ( see e.g. com.kentyou.featurelauncher.impl.repository.RemoteArtifactRepositoryImplTest.testCreateRemoteArtifactRepository() ); perhaps API should be modified and that method accept additional argument Path which specifies path to local repository ? i.e. instead of createRepository(URI, Map<String, Object>) that method would become createRepository(URI, Path, Map<String, Object>) ?

• there's a conflict between org.osgi.service.featurelauncher.FeatureLauncher.LaunchBuilder.withFrameworkProperties(Map<String, Object>) and org.osgi.framework.launch.FrameworkFactory.newFramework(Map<String, String>) - I added com.kentyou.featurelauncher.impl.FeatureLauncherImpl.LaunchBuilderImpl.convertFrameworkProperties(Map<String, Object>) due to this; should this stay as is or API ( org.osgi.service.featurelauncher.FeatureLauncher.LaunchBuilder.withFrameworkProperties(Map<String, Object>) ) should be changed ?

• regarding managing configurations:

  • configurations can only be installed once 'ConfigurationAdmin' service is available, and that is only once framework is started, so section "160.4.3.4" title ( "Installing bundles and configurations" ) is misleading - vide comment @ com.kentyou.featurelauncher.impl.FeatureLauncherImpl.LaunchBuilderImpl.launchFramework();

  • whose responsibility is it to provide configuration admin service implementation ( e.g. 'org.apache.felix.configadmin' ) ? currently it is provided via feature's bundles, as first artifact on that list ( see e.g. console-webconsole-feature.json feature definition @ src/test/resources/features );

[timothyjward]

In addition to PR review comments you may have Tim, here's few items I wanted to mention:

• I added com.kentyou.featurelauncher.impl.repository.EnhancedArtifactRepository as org.osgi.service.featurelauncher.repository.ArtifactRepository only defines getArtifact(ID) method which returns InputStream, while having method which returns Path to artifact ( like getArtifactPath(ID) added to EnhancedArtifactRepository ) is very useful as well; shouldn't that be worked into the API ( org.osgi.service.featurelauncher.repository.ArtifactRepository ) or leave as is ( i.e. have EnhancedArtifactRepository which extends ArtifactRepository ) ?
• that additional getArtifactPath(ID) method made it much easier to utilize Java's built-in class loader ( URLClassLoader ), and avoid creating custom class loader which handles JARs from InputStream; is that acceptable or instead of URLClassLoader create custom class loader which handles JARs from InputStream ?

The issue here is that it forces a download to a local path, which isn’t something we want to require from everyone implementing ArtifactRepository (users can supply their own). I can look and have a think, but it’s likely the spec won’t change here as we prioritise user convenience over implementation convenience.

• I wasn't sure if logic for selecting framework ( com.kentyou.featurelauncher.impl.FrameworkFactoryLocator.selectFrameworkFactory(FeatureExtension, List<ArtifactRepository>) ) should be repackaged to specialized FeatureExtensionHandler, e.g. LaunchFrameworkFeatureExtensionHandlerImpl - but, the only method defined on FeatureExtensionHandler returns Feature.. and this select framework "Feature extension" does not require pre-processing, it is FrameworkFactory that must be returned; please clarify;

This is an implementation choice. There will need to be a handler for the spec defined extension so features that use it can start, so it does sound like a good idea to implement it this way. How you communicate the selected framework back is up to you. You could store a copy of the extension in the launch and ask it?

• since path to local Maven repository is also needed to create remote repository, as a ( temporary ? ) work around this problem, I had to use configuration properties passed org.osgi.service.featurelauncher.repository.ArtifactRepositoryFactory.createRepository(URI, Map<String, Object>) to provide this path ( see e.g. com.kentyou.featurelauncher.impl.repository.RemoteArtifactRepositoryImplTest.testCreateRemoteArtifactRepository() ); perhaps API should be modified and that method accept additional argument Path which specifies path to local repository ? i.e. instead of createRepository(URI, Map<String, Object>) that method would become createRepository(URI, Path, Map<String, Object>) ?

could you not just use a temporary directory if no path is passed in the properties? Do we need a definition for the property name in the specification?

• there's a conflict between org.osgi.service.featurelauncher.FeatureLauncher.LaunchBuilder.withFrameworkProperties(Map<String, Object>) and org.osgi.framework.launch.FrameworkFactory.newFramework(Map<String, String>) - I added com.kentyou.featurelauncher.impl.FeatureLauncherImpl.LaunchBuilderImpl.convertFrameworkProperties(Map<String, Object>) due to this; should this stay as is or API ( org.osgi.service.featurelauncher.FeatureLauncher.LaunchBuilder.withFrameworkProperties(Map<String, Object>) ) should be changed ?

We should change the API. There’s no point letting people pass nonsense!

• regarding managing configurations:

  • configurations can only be installed once 'ConfigurationAdmin' service is available, and that is only once framework is started, so section "160.4.3.4" title ( "Installing bundles and configurations" ) is misleading - vide comment @ com.kentyou.featurelauncher.impl.FeatureLauncherImpl.LaunchBuilderImpl.launchFramework();

The configurations can only be created then, but the listener can (and should) be created after the framework has been initialised, but before bundles are started.

• whose responsibility is it to provide configuration admin service implementation ( e.g. 'org.apache.felix.configadmin' ) ? currently it is provided via feature's bundles, as first artifact on that list ( see e.g. console-webconsole-feature.json feature definition @ src/test/resources/features );

This should come from the feature. If the user doesn’t include one, but does include configurations, then the launch should time out and fail.

[ideas-into-software]

In addition to PR review comments you may have Tim, here's few items I wanted to mention:

• I added com.kentyou.featurelauncher.impl.repository.EnhancedArtifactRepository as org.osgi.service.featurelauncher.repository.ArtifactRepository only defines getArtifact(ID) method which returns InputStream, while having method which returns Path to artifact ( like getArtifactPath(ID) added to EnhancedArtifactRepository ) is very useful as well; shouldn't that be worked into the API ( org.osgi.service.featurelauncher.repository.ArtifactRepository ) or leave as is ( i.e. have EnhancedArtifactRepository which extends ArtifactRepository ) ?
• that additional getArtifactPath(ID) method made it much easier to utilize Java's built-in class loader ( URLClassLoader ), and avoid creating custom class loader which handles JARs from InputStream; is that acceptable or instead of URLClassLoader create custom class loader which handles JARs from InputStream ?

The issue here is that it forces a download to a local path, which isn’t something we want to require from everyone implementing ArtifactRepository (users can supply their own). I can look and have a think, but it’s likely the spec won’t change here as we prioritise user convenience over implementation convenience.

• I wasn't sure if logic for selecting framework ( com.kentyou.featurelauncher.impl.FrameworkFactoryLocator.selectFrameworkFactory(FeatureExtension, List<ArtifactRepository>) ) should be repackaged to specialized FeatureExtensionHandler, e.g. LaunchFrameworkFeatureExtensionHandlerImpl - but, the only method defined on FeatureExtensionHandler returns Feature.. and this select framework "Feature extension" does not require pre-processing, it is FrameworkFactory that must be returned; please clarify;

This is an implementation choice. There will need to be a handler for the spec defined extension so features that use it can start, so it does sound like a good idea to implement it this way. How you communicate the selected framework back is up to you. You could store a copy of the extension in the launch and ask it?

• since path to local Maven repository is also needed to create remote repository, as a ( temporary ? ) work around this problem, I had to use configuration properties passed org.osgi.service.featurelauncher.repository.ArtifactRepositoryFactory.createRepository(URI, Map<String, Object>) to provide this path ( see e.g. com.kentyou.featurelauncher.impl.repository.RemoteArtifactRepositoryImplTest.testCreateRemoteArtifactRepository() ); perhaps API should be modified and that method accept additional argument Path which specifies path to local repository ? i.e. instead of createRepository(URI, Map<String, Object>) that method would become createRepository(URI, Path, Map<String, Object>) ?

could you not just use a temporary directory if no path is passed in the properties? Do we need a definition for the property name in the specification?

• there's a conflict between org.osgi.service.featurelauncher.FeatureLauncher.LaunchBuilder.withFrameworkProperties(Map<String, Object>) and org.osgi.framework.launch.FrameworkFactory.newFramework(Map<String, String>) - I added com.kentyou.featurelauncher.impl.FeatureLauncherImpl.LaunchBuilderImpl.convertFrameworkProperties(Map<String, Object>) due to this; should this stay as is or API ( org.osgi.service.featurelauncher.FeatureLauncher.LaunchBuilder.withFrameworkProperties(Map<String, Object>) ) should be changed ?

We should change the API. There’s no point letting people pass nonsense!

• regarding managing configurations:

  • configurations can only be installed once 'ConfigurationAdmin' service is available, and that is only once framework is started, so section "160.4.3.4" title ( "Installing bundles and configurations" ) is misleading - vide comment @ com.kentyou.featurelauncher.impl.FeatureLauncherImpl.LaunchBuilderImpl.launchFramework();

The configurations can only be created then, but the listener can (and should) be created after the framework has been initialised, but before bundles are started.

• whose responsibility is it to provide configuration admin service implementation ( e.g. 'org.apache.felix.configadmin' ) ? currently it is provided via feature's bundles, as first artifact on that list ( see e.g. console-webconsole-feature.json feature definition @ src/test/resources/features );

This should come from the feature. If the user doesn’t include one, but does include configurations, then the launch should time out and fail.

I will address your comment regarding remote repository implementation first:

• we discussed last Monday use of "Eclipse Aether library" ( currently developed as "Maven Artifact Resolver" ), which Maven uses underneath;

• there is local and remote repository

  • both local and remote repoisitory should be configured and passed to FeatureLauncher;

  • if artifact is not found in local repository, only then remote repository is queried - just like in e.g. com.kentyou.featurelauncher.impl.FeatureLauncherImplTest.testLaunchFeatureWithLaunchFrameworkExtension();

• "Eclipse Aether" / "Maven Artifact Resolver" REQUIRES having local repository is defined before remote repository session can even be started - otherwise it fails with "java.lang.IllegalStateException: No local repository manager or local repositories set on session"; see screenshot attached as well as https://maven.apache.org/resolver/creating-a-repository-system-session.html;

• if remote artifact repository has path to local repository set to temporary folder underneath and it is different from local artifact repository is using, those will never be in sync, and artifacts will always have to be fetched remotely;

• the two should definitely be the same, so artifacts are only fetched from remote if they do not already exist in local repository ( i.e. were not already fetched from remote previously ), just like in e.g. com.kentyou.featurelauncher.impl.FeatureLauncherImplTest.testLaunchFeatureWithLaunchFrameworkExtension();

Hopefully, above mentioned puts the other choices I made in implementations shared via this PR more understandable.

I proposed adding getArtifactPath(ID) as this allows utilizing Java's built-in URLClassLoader, asking if that is acceptable, considering above mentioned. Otherwise, custom ClassLoader would need to be implemented, which uses your getArtifact(ID) and operates on InputStream directly .. I could obviously write that artifact from input stream to temporary directory and construct URLClassLoader anyway, but having artifact in local repository already makes it much easier and does not add overhead of writing those same artifacts in temporary folder just for the purpose of constructing class loader, etc. And, again, having both local and remote repository configured ( like in e.g. com.kentyou.featurelauncher.impl.FeatureLauncherImplTest.testLaunchFeatureWithLaunchFrameworkExtension() ), and remote repository using same storage for resolved and downloaded artifacts ( since local storage has to be defined for remote repo anyway ), removes a lot of additional overhead and makes it much easier.

And, lastly, regarding com.kentyou.featurelauncher.impl.FrameworkFactoryLocator.selectFrameworkFactory(FeatureExtension, List<ArtifactRepository>) - selecting framework does not process Feature in any way and must return FrameworkFactory back to com.kentyou.featurelauncher.impl.FrameworkFactoryLocator.locateFrameworkFactory(Feature, List<ArtifactRepository>) so other branches required in "160.4.3.2" can be implemented; that is why I made this choice.

We can discuss this further on Monday, but hopefully with this additional info I just shared via this and other comments you understand why I made these choices.. and if I am to change something, all this must be taken into consideration, as in most cases that is how I initially wanted to implement these, but it did not work or made it much more ( and unnecessarily so ) difficult / time-consuming.

---

[ideas-into-software]

Most recent commit ( 0c1ac89 ) contains changes discussed during PR #23 review plus refactoring / fixes / improvements:

• com.kentyou.featurelauncher.impl.FeatureLauncherImpl

  • refactor related to configuration admin - i.e. create configuration admin tracker using Framework's bundle context once bundles are installed, wait for configuration admin tracker service once framework is started;
  • extracted deleteFrameworkStorageArea method to FileSystemUtil and updated call to this method;
  • added comment in cleanup(Framework) method regarding CountDownLatch as TODO item;

• com.kentyou.featurelauncher.impl.FrameworkFactoryLocator

  • defined LaunchFrameworkFeatureExtensionHandler interface which extends FeatureExtensionHandler;
  • refactored selectFrameworkFactory(FeatureExtension, List<ArtifactRepository) and related methods to LaunchFrameworkFeatureExtensionHandlerImpl, which implements LaunchFrameworkFeatureExtensionHandler, and updated calls from FrameworkFactoryLocator;
  • IMHO, this is a huge abuse of API defined by FeatureExtensionHandler and does not look well, it needs to expose 3 additional public methods and make the main method defined on FeatureExtensionHandler a NOP;

• com.kentyou.featurelauncher.impl.FeatureConfigurationManager

  • moved initialization and starting of service tracker to constructor;
  • exposed waitForService method;
  • refactored required for reflection to work after these changes;

• com.kentyou.featurelauncher.impl.repository.EnhancedArtifactRepository

  • renamed to FileSystemArtifactRepository;
  • updated its Javadoc;
  • added new method getLocalRepositoryPath() which returns Path to local artifact repository;

• com.kentyou.featurelauncher.impl.repository.RemoteArtifactRepositoryImpl

  • removed requirement to define path to local repository - if not defined, temporary directory will be created;
  • added test case for RemoteArtifactRepositoryImpl without passing path to local repository @ com.kentyou.featurelauncher.impl.repository.RemoteArtifactRepositoryImplTest.testCreateRemoteArtifactRepositoryWithTemporaryLocalArtifactRepository()
  • removed comment related to EnhancedArtifactRepository;
  • added implementation of getLocalRepositoryPath method;

• com.kentyou.featurelauncher.impl.repository.LocalArtifactRepositoryImpl

  • removed comment related to EnhancedArtifactRepository;
  • added implementation of getLocalRepositoryPath method;

• com.kentyou.featurelauncher.impl.repository.ArtifactRepositoryFactoryImpl

  • removed validation of LOCAL_ARTIFACT_REPOSITORY_PATH - instead, default behaviour is to create local repository in temporary directory ( see com.kentyou.featurelauncher.impl.repository.RemoteArtifactRepositoryImpl above );

Therefore, resolving all conversations on this PR review.

[timothyjward]

I think this is ready to merge now.

• com.kentyou.featurelauncher.impl.FrameworkFactoryLocator

  • defined LaunchFrameworkFeatureExtensionHandler interface which extends FeatureExtensionHandler;
  • refactored selectFrameworkFactory(FeatureExtension, List<ArtifactRepository) and related methods to LaunchFrameworkFeatureExtensionHandlerImpl, which implements LaunchFrameworkFeatureExtensionHandler, and updated calls from FrameworkFactoryLocator;
  • IMHO, this is a huge abuse of API defined by FeatureExtensionHandler and does not look well, it needs to expose 3 additional public methods and make the main method defined on FeatureExtensionHandler a NOP;

I think that this approach will make a lot more sense once decorator and extension processing are fully implemented. If someone defines a MANDATORY extension in their feature then the LaunchBuilder must have a FeatureExtensionHandler for that extension. If it doesn't then launching must fail. We therefore must have a handler for this extension built into the implementation. Once the handler is being called the processing of the extension entries could happen as part of the handle method, storing the result for later. No changes required for the moment though.

[timothyjward]

@ideas-into-software - can this be merged?

[ideas-into-software]

@ideas-into-software - can this be merged?

I will merge it once new PR is delivered, within next 12 hours or so. Is that OK ? It will contain more of the stuff you need for the demo, including OSGi integration tests and Feature Runtime Service.

[timothyjward]

Specification feedback has been merged in osgi/osgi@ad6256d including:

• API change for launch properties to match the Framework Launcher
• Clarity around permitted value types for variables
• JavaDoc and specification text clarifying that multiple ArtifactRepository, FeatureDecorator and FeatureExtensionHandler instances may be registered.