You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by sjcorbett <gi...@git.apache.org> on 2014/06/30 19:37:33 UTC

[GitHub] incubator-brooklyn pull request: Initial OSGI work

GitHub user sjcorbett opened a pull request:

    https://github.com/apache/incubator-brooklyn/pull/31

    Initial OSGI work

    Towards BROOKLYN-13.
    * Adds @ahgittin's initial work with Felix
    * Updates catalog.xml to load and store bundle URLs under a `<libraries>` element.
    * Adds helpers for loading classes from bundles to `BrooklynEntityClassResolver`

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/sjcorbett/incubator-brooklyn yaml-context

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/incubator-brooklyn/pull/31.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #31
    
----
commit e4e1a55037dbb6be02367ed355dd7f70358617cb
Author: Alex Heneveld <al...@cloudsoftcorp.com>
Date:   2014-06-11T03:27:57Z

    osgi wip

commit fd763b05d3e8a7b303b760d306fe591096a6e20a
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-26T13:28:24Z

    Ground work to load classes from bundles in blueprints

commit 2dcc8e6c041d6a93e716e66b05c0a1c274392a38
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-28T16:11:16Z

    Adds context element to catalogue entries

commit 9e28648f67e1eb57eea756dff6791342d53b412c
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-30T11:54:44Z

    Fix infinite recursion in Osgis.getBundle

commit e9efa0b7b1e2b264c3ca8bc9ed591162257a4a18
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-30T12:19:51Z

    Incorporate loading from bundles into BrooklynEntityClassResolver

commit 69d42234d17d32344432db5ceda4e1cda6a7f6f5
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-30T14:23:35Z

    Adds version to catalogue items and tests deserialisation

commit 4f61ad6f0bcc6ec58606a750cde4fb2b65fd4088
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-30T15:07:10Z

    Rename catalog context to libraries

commit c5478e089ffcced93ea37a17b97b0c900b70dbad
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-30T16:23:37Z

    Deprecate BrooklynCatalog.addItem and remove addToClasspath

commit 28c8bcac069299ebf376fdadebdd4719ae8d304b
Author: Sam Corbett <sa...@cloudsoftcorp.com>
Date:   2014-06-30T16:24:33Z

    Note problem with BrooklynAssemblyTemplateInstantiator catalog path

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14378770
  
    --- Diff: core/src/main/java/brooklyn/catalog/internal/CatalogItemDtoAbstract.java ---
    @@ -1,14 +1,18 @@
     package brooklyn.catalog.internal;
     
    +import javax.annotation.Nonnull;
    +
     import brooklyn.catalog.CatalogItem;
     
     public abstract class CatalogItemDtoAbstract<T> implements CatalogItem<T> {
    -    
    +
         String id;
         String type;
         String name;
         String description;
         String iconUrl;
    +    String version;
    --- End diff --
    
    Longer term, would be nice to have a Version class that wraps the string - it would support comparisons etc.
    Ideally we'd also use some third part library that correctly implements semantic versioning.
    Alternatively, if we used OSGi versioning (different semantics as I recall, such as `1.20 < 1.3`) then maybe Felix has an appropriate comparator.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14484584
  
    --- Diff: core/src/main/java/brooklyn/catalog/internal/CatalogLibrariesDo.java ---
    @@ -0,0 +1,22 @@
    +package brooklyn.catalog.internal;
    +
    +import java.util.List;
    +
    +import com.google.common.base.Preconditions;
    +
    +import brooklyn.catalog.CatalogItem;
    +
    +public class CatalogLibrariesDo implements CatalogItem.CatalogItemLibraries {
    +
    +    private final CatalogLibrariesDto librariesDto;
    +
    +    public CatalogLibrariesDo(CatalogLibrariesDto librariesDto) {
    +        this.librariesDto = Preconditions.checkNotNull(librariesDto, "librariesDto");
    --- End diff --
    
    in fact libraries is often null due to the `CatalogItemAbstractDto` constructor methods.  i think we should remove the `@Nonnull` and allow null simply to mean no special libraries defined.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14378460
  
    --- Diff: api/src/main/java/brooklyn/catalog/BrooklynCatalog.java ---
    @@ -4,45 +4,49 @@
     
     public interface BrooklynCatalog {
     
    -    /** finds the item with the given ID, or null if not found */
    +    /** @return The item with the given ID, or null if not found */
         CatalogItem<?> getCatalogItem(String id);
    -    
    +
    +
         /** variant of {@link #getCatalogItem(String)} which checks (and casts) type for convenience
          * (returns null if type does not match) */
         <T> CatalogItem<T> getCatalogItem(Class<T> type, String id);
    -    
    -    /** returns all items in the catalog */
    +
    +    /** @return All items in the catalog */
         <T> Iterable<CatalogItem<T>> getCatalogItems();
    +
         /** convenience for filtering items in the catalog; see CatalogPredicates for useful filters */
         <T> Iterable<CatalogItem<T>> getCatalogItems(Predicate<? super CatalogItem<T>> filter);
     
    -    /** returns the classloader which should be used to load classes and entities;
    +    /** @return The classloader which should be used to load classes and entities;
          * this includes all the catalog's classloaders in the right order */
         public ClassLoader getRootClassLoader();
    -    
    +
         /** throws exceptions if any problems */
         <T> Class<? extends T> loadClass(CatalogItem<T> item);
         <T> Class<? extends T> loadClassByType(String typeName, Class<T> typeClass);
    -    
    -    /** adds an item to the 'manual' catalog;
    -     * this does not update the classpath or have a record to the java Class,
    -     * so callers of this method will typically also need to call 
    -     * {@link #addToClasspath(String)} or {@link #addToClasspath(ClassLoader)} */
    +
    +    /**
    +     * adds an item to the 'manual' catalog;
    +     * this does not update the classpath or have a record to the java Class
    +     *
    +     * @deprecated since 0.7.0 Construct catalogs with OSGi bundles instead
    +     */
    +    @Deprecated
    --- End diff --
    
    Will we instead pass the yaml in, so have a `addItem(String yaml)`?
    Or should we also / instead support `addItem(CatalogItem)` where the CatalogItem can include the yaml, as well as the extracted metadata about it?
    (Note it's currently not obvious how to create a CatalogItem to use this - one has to instantiate+set a CatalogEntityItemDto, which is buried in an `.internal` package.)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14379176
  
    --- Diff: core/src/main/java/brooklyn/management/ha/OsgiManager.java ---
    @@ -0,0 +1,64 @@
    +package brooklyn.management.ha;
    +
    +import java.io.File;
    +
    +import org.osgi.framework.BundleException;
    +import org.osgi.framework.launch.Framework;
    +
    +import brooklyn.config.BrooklynServerConfig;
    +import brooklyn.config.ConfigKey;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.guava.Maybe;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.osgi.Osgis;
    +
    +public class OsgiManager {
    +
    +    public static final ConfigKey<Boolean> USE_OSGI = BrooklynServerConfig.USE_OSGI;
    +    
    +    /* see Osgis for info on starting framework etc */
    +    
    +    protected Framework framework;
    +    protected File osgiTempDir;
    +    
    +    public void start() {
    +        try {
    +            // TODO any extra startup args?
    +            // TODO dir to come from brooklyn properties;
    +            // note dir must be different for each if starting multiple instances
    +            osgiTempDir = Os.newTempDir("brooklyn-osgi-cache");
    +            framework = Osgis.newFrameworkStarted(osgiTempDir.getAbsolutePath(), false, MutableMap.of());
    +            
    +        } catch (Exception e) {
    +            throw Exceptions.propagate(e);
    +        }
    +    }
    +
    +    public void stop() {
    +        try {
    +            if (framework!=null)
    +                framework.stop();
    +        } catch (BundleException e) {
    +            throw Exceptions.propagate(e);
    +        }
    +        osgiTempDir = Os.deleteRecursively(osgiTempDir).asNullOrThrowing();
    +        framework = null;
    +    }
    +
    +    // TODO: throws BundleException appropriate?
    +    public void registerBundle(String bundleUrl) throws BundleException {
    +        Osgis.install(framework, bundleUrl);
    +    }
    +
    +    // TODO: Handle .get failing
    +    public <T> Maybe<Class<T>> tryResolveClass(String bundleUrl, String type) {
    +        try {
    +            Class<T> clazz = (Class<T>) Osgis.getBundle(framework, bundleUrl).get().loadClass(type);
    --- End diff --
    
    `loadClass(type)` will throw `ClassNotFoundException` if not found.
    Better to just catch that rather than generic Exception?
    e.g. if bundle name was invalid then fine to throw exception rather than just returning `Maybe.absent()`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14379322
  
    --- Diff: core/src/main/java/brooklyn/util/osgi/Osgis.java ---
    @@ -0,0 +1,151 @@
    +package brooklyn.util.osgi;
    +
    +import java.io.BufferedReader;
    +import java.io.InputStream;
    +import java.io.InputStreamReader;
    +import java.net.URL;
    +import java.util.List;
    +import java.util.Map;
    +
    +import org.apache.felix.framework.FrameworkFactory;
    +import org.osgi.framework.Bundle;
    +import org.osgi.framework.BundleException;
    +import org.osgi.framework.Constants;
    +import org.osgi.framework.Version;
    +import org.osgi.framework.launch.Framework;
    +import org.slf4j.Logger;
    +import org.slf4j.LoggerFactory;
    +
    +import brooklyn.util.ResourceUtils;
    +import brooklyn.util.collections.MutableList;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.guava.Maybe;
    +
    +import com.google.common.annotations.Beta;
    +import com.google.common.base.Joiner;
    +import com.google.common.base.Predicate;
    +import com.google.common.base.Predicates;
    +
    +/** 
    + * utilities for working with osgi.
    + * osgi support is in early days (June 2014) so this class is beta, subject to change,
    + * particularly in how framework is started and bundles installed.
    + * 
    + * @since 0.7.0  */
    +@Beta
    +public class Osgis {
    +
    +    private static final Logger LOG = LoggerFactory.getLogger(Osgis.class);
    +
    +    public static List<Bundle> getBundlesByName(Framework framework, String symbolicName, Predicate<Version> versionMatcher) {
    +        List<Bundle> result = MutableList.of();
    +        for (Bundle b: framework.getBundleContext().getBundles()) {
    +            if (symbolicName.equals(b.getSymbolicName()) && versionMatcher.apply(b.getVersion())) {
    +                result.add(b);
    +            }
    +        }
    +        return result;
    +    }
    +
    +    public static List<Bundle> getBundlesByName(Framework framework, String symbolicName) {
    +        return getBundlesByName(framework, symbolicName, Predicates.<Version>alwaysTrue());
    +    }
    +
    +    /**
    +     * Tries to find a bundle in the given framework with name matching either `name' or `name:version'.
    +     */
    +    public static Maybe<Bundle> getBundle(Framework framework, String symbolicNameOptionallyWithVersion) {
    +        String[] parts = symbolicNameOptionallyWithVersion.split(":");
    +        Maybe<Bundle> result = Maybe.absent("No bundles matching "+symbolicNameOptionallyWithVersion);
    +        if (parts.length == 2) {
    +            result = getBundle(framework, parts[0], parts[1]);
    +        } else if (parts.length == 1) {
    +            List<Bundle> matches = getBundlesByName(framework, symbolicNameOptionallyWithVersion);
    +            if (!matches.isEmpty()) {
    +                result = Maybe.of(matches.iterator().next());
    --- End diff --
    
    Perhaps add a TODO about getting the latest stable version, rather than just first in list?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14398853
  
    --- Diff: core/src/main/java/brooklyn/catalog/internal/CatalogItemDtoAbstract.java ---
    @@ -1,14 +1,18 @@
     package brooklyn.catalog.internal;
     
    +import javax.annotation.Nonnull;
    +
     import brooklyn.catalog.CatalogItem;
     
     public abstract class CatalogItemDtoAbstract<T> implements CatalogItem<T> {
    -    
    +
         String id;
         String type;
         String name;
         String description;
         String iconUrl;
    +    String version;
    --- End diff --
    
    If we go with OSGi versioning we can use Felix's implementation of [org.osgi.framework.Version](http://www.osgi.org/javadoc/r5/core/org/osgi/framework/Version.html) class, which implements `Comparable<Version>`. There's also a `VersionRange` class.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/incubator-brooklyn/pull/31


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14379065
  
    --- Diff: core/src/main/java/brooklyn/management/ha/OsgiManager.java ---
    @@ -0,0 +1,64 @@
    +package brooklyn.management.ha;
    +
    +import java.io.File;
    +
    +import org.osgi.framework.BundleException;
    +import org.osgi.framework.launch.Framework;
    +
    +import brooklyn.config.BrooklynServerConfig;
    +import brooklyn.config.ConfigKey;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.guava.Maybe;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.osgi.Osgis;
    +
    +public class OsgiManager {
    +
    +    public static final ConfigKey<Boolean> USE_OSGI = BrooklynServerConfig.USE_OSGI;
    +    
    +    /* see Osgis for info on starting framework etc */
    +    
    +    protected Framework framework;
    +    protected File osgiTempDir;
    +    
    +    public void start() {
    +        try {
    +            // TODO any extra startup args?
    +            // TODO dir to come from brooklyn properties;
    +            // note dir must be different for each if starting multiple instances
    +            osgiTempDir = Os.newTempDir("brooklyn-osgi-cache");
    +            framework = Osgis.newFrameworkStarted(osgiTempDir.getAbsolutePath(), false, MutableMap.of());
    +            
    +        } catch (Exception e) {
    +            throw Exceptions.propagate(e);
    +        }
    +    }
    +
    +    public void stop() {
    +        try {
    +            if (framework!=null)
    +                framework.stop();
    +        } catch (BundleException e) {
    +            throw Exceptions.propagate(e);
    +        }
    +        osgiTempDir = Os.deleteRecursively(osgiTempDir).asNullOrThrowing();
    +        framework = null;
    +    }
    +
    +    // TODO: throws BundleException appropriate?
    +    public void registerBundle(String bundleUrl) throws BundleException {
    +        Osgis.install(framework, bundleUrl);
    +    }
    +
    +    // TODO: Handle .get failing
    --- End diff --
    
    Should we return `Maybe.<Class<T>>absent()` if `getBundle()` returns absent?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] incubator-brooklyn pull request: Initial OSGI work

Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:

    https://github.com/apache/incubator-brooklyn/pull/31#discussion_r14403209
  
    --- Diff: core/src/main/java/brooklyn/management/ha/OsgiManager.java ---
    @@ -0,0 +1,64 @@
    +package brooklyn.management.ha;
    +
    +import java.io.File;
    +
    +import org.osgi.framework.BundleException;
    +import org.osgi.framework.launch.Framework;
    +
    +import brooklyn.config.BrooklynServerConfig;
    +import brooklyn.config.ConfigKey;
    +import brooklyn.util.collections.MutableMap;
    +import brooklyn.util.exceptions.Exceptions;
    +import brooklyn.util.guava.Maybe;
    +import brooklyn.util.os.Os;
    +import brooklyn.util.osgi.Osgis;
    +
    +public class OsgiManager {
    +
    +    public static final ConfigKey<Boolean> USE_OSGI = BrooklynServerConfig.USE_OSGI;
    +    
    +    /* see Osgis for info on starting framework etc */
    +    
    +    protected Framework framework;
    +    protected File osgiTempDir;
    +    
    +    public void start() {
    +        try {
    +            // TODO any extra startup args?
    +            // TODO dir to come from brooklyn properties;
    +            // note dir must be different for each if starting multiple instances
    +            osgiTempDir = Os.newTempDir("brooklyn-osgi-cache");
    +            framework = Osgis.newFrameworkStarted(osgiTempDir.getAbsolutePath(), false, MutableMap.of());
    +            
    +        } catch (Exception e) {
    +            throw Exceptions.propagate(e);
    +        }
    +    }
    +
    +    public void stop() {
    +        try {
    +            if (framework!=null)
    +                framework.stop();
    +        } catch (BundleException e) {
    +            throw Exceptions.propagate(e);
    +        }
    +        osgiTempDir = Os.deleteRecursively(osgiTempDir).asNullOrThrowing();
    +        framework = null;
    +    }
    +
    +    // TODO: throws BundleException appropriate?
    +    public void registerBundle(String bundleUrl) throws BundleException {
    +        Osgis.install(framework, bundleUrl);
    +    }
    +
    +    // TODO: Handle .get failing
    --- End diff --
    
    Done in 30093bb


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---