[
https://issues.apache.org/jira/browse/IGNITE-1270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15027062#comment-15027062
]
Raúl Kripalani edited comment on IGNITE-1270 at 11/25/15 4:33 PM:
------------------------------------------------------------------
Hi Romain,
Thanks for your input! Here's my feedback:
bq. Why you introduce the constrain of having only one ignite instance per jvm?
Unfortunately many Ignite variables/classes are in the static namespace, i.e.
singletons. While this is OK for most non-OSGi containers out there, because
the static classes will be loaded as many times as classloaders, this does not
work in OSGi, because the static classes are only loaded once, and only once,
per container. For example, have a look at {{IgniteUtils#gridClassLoader}}. In
a web container with multiple WARs, each WAR will load its own copy of
IgniteUtils. However, in an OSGi container that's not the case, therefore if we
load multiple grids, the gridClassLoader (and all such variables) will not
differentiate between one grid and another.
*IgniteOsgiUtils*
bq. I think this class should be package protected or in an private package. I
see it more as an internal implementation detail and client should not be
allowed to depends / use.
I didn't make the class private/protected nor static (even though it's a helper
class for now) because in the future it may be necessary from ignite-core (e.g.
serializers) to find out if the current environment is OSGi or not, or get the
appropriate classloader, etc. Keeping it like this allows us to use the
{{IgniteComponentType}} lookup mechanism in the future.
bq. gridCount()
I agree too. Grid in my head is something composed of many nodes. But in
Ignite, the term is used to refer to a single Ignite instance (have a look at
the unit tests).
bq. classloaders()
Good point. I've made {{classloaders()}} protected. When we support multiple
instances, we can add a {{public static ClassLoader getClassLoader(Ignite
ignite)}} method. Until then, exposing an API for this is useless.
bq. Why don't you use the parent child classloader delegation?
Because in OSGi, classloaders are connected in a graph fashion, not in a tree
pattern. The parent classloader is useless here.
bq. Why are you going first to the bundle and then to the class loader. Maybe
you should put the given class loader as a parent and then fallback to the
bundle, no? Because this bundle will import the Ignite package and therefore
its classloader should be able to see some of the ignite classes.
I've followed the model that has worked pretty well in Apache Camel. In pure
OSGi, bundles are always resolved from within the classspace of the current
bundle. This is what the user expects. That's why the bundle receives priority
#1 in my implementation.
bq. Do you plan to support the dynamic addition and removal of bundles? This
dynamicity may impact the nonResolvable and resolved Set. As some classes may
become resolvable and some other not or in a different way [...]
Yes, I agree with you entirely. It is in my roadmap to manage the caches
appropriately as bundles come and go via a {{BundleTracker}}. However, this is
only necessary in a changing environment. For now I don't see it as a blocker,
but it's definitely something to be done in Ignite 1.6 or even 1.5.1.
*IgniteOsgiContextActivator*
bq. Maybe the Abstract key word should be used in the name to make it crystal
clear
Agree, thanks for the note.
bq. You should make the start(BundleContext context) and stop(BundleContext
context) final in order to avoid the hack I mention above.
Hmmm, ok. But in exchange I have introduced callback methods {{preStart}},
{{postStart}}, {{preStop}}, {{postStop}} to give users some flexibility to
introduce custom logic.
*Maven*
bq. In you effort to extract the version maybe we can add a new jira ticket to
also use the dependencyManagement section to avoid to use the version
properties everywhere?
I think it's overkill to add all those dependencies in dependencyManagement,
when most of them are only used in one module. I _could_ see shared
dependencies being moved to dependencyManagement, but I don't like having mixed
criteria and further development overhead. No one is going to remember in the
future to promote an ad-hoc dependency to dependencyManagement if they
introduce a dependency that happens to be used elsewhere... So I rather keep it
as it is. It's already a huge step forward to have dependency versions in
properties, as it facilitates tracking and upgrade management big time.
---
I have pushed my changes to the branch. Have a look again and let me know what
you think.
Thanks for your constructive comments!
Regards,
Raúl.
was (Author: raulvk):
Hi Romain,
Thanks for your input! Here's my feedback:
bq. Why you introduce the constrain of having only one ignite instance per jvm?
Unfortunately many Ignite variables/classes are at the static namespace, i.e.
singletons. While this is OK for most non-OSGi containers out there, because
the static classes will be loaded as many times as classloaders, this does not
work in OSGi, because the static classes are only loaded once, and only once,
per container. For example, have a look at {{IgniteUtils#gridClassLoader}}. In
a web container with multiple WARs, each WAR will load its own copy of
IgniteUtils. However, in an OSGi container that's not the case, therefore if we
load multiple grids, the gridClassLoader (and all such variables) will not
differentiate between one grid and another.
*IgniteOsgiUtils*
bq. I think this class should be package protected or in an private package. I
see it more as an internal implementation detail and client should not be
allowed to depends / use.
I didn't make the class private/protected nor static (even though it's a helper
class for now) because in the future it may be necessary from ignite-core (e.g.
serializers) to find out if the current environment is OSGi or not, or get the
appropriate classloader, etc. Keeping it like this allows us to use the
{{IgniteComponentType}} lookup mechanism in the future.
bq. gridCount()
I agree too. Grid in my head is something composed of many nodes. But in
Ignite, the term is used to refer to a single Ignite instance (have a look at
the unit tests).
bq. classloaders()
Good point. I've made {{classloaders()}} protected. When we support multiple
instances, we can add a {{public static ClassLoader getClassLoader(Ignite
ignite)}} method. Until then, exposing an API for this is useless.
bq. Why don't you use the parent child classloader delegation?
Because in OSGi, classloaders are connected in a graph fashion, not in a tree
pattern. The parent classloader is useless here.
bq. Why are you going first to the bundle and then to the class loader. Maybe
you should put the given class loader as a parent and then fallback to the
bundle, no? Because this bundle will import the Ignite package and therefore
its classloader should be able to see some of the ignite classes.
I've followed the model that has worked pretty well in Apache Camel. In pure
OSGi, bundles are always resolved from within the classspace of the current
bundle. This is what the user expects. That's why the bundle receives priority
#1 in my implementation.
bq. Do you plan to support the dynamic addition and removal of bundles? This
dynamicity may impact the nonResolvable and resolved Set. As some classes may
become resolvable and some other not or in a different way [...]
Yes, I agree with you entirely. It is in my roadmap to manage the caches
appropriately as bundles come and go via a {{BundleTracker}}. However, this is
only necessary in a changing environment. For now I don't see it as a blocker,
but it's definitely something to be done in Ignite 1.6 or even 1.5.1.
*IgniteOsgiContextActivator*
bq. Maybe the Abstract key word should be used in the name to make it crystal
clear
Agree, thanks for the note.
bq. You should make the start(BundleContext context) and stop(BundleContext
context) final in order to avoid the hack I mention above.
Hmmm, ok. But in exchange I have introduced callback methods {{preStart}},
{{postStart}}, {{preStop}}, {{postStop}} to give users some flexibility to
introduce custom logic.
*Maven*
bq. In you effort to extract the version maybe we can add a new jira ticket to
also use the dependencyManagement section to avoid to use the version
properties everywhere?
I think it's overkill to add all those dependencies in dependencyManagement,
when most of them are only used in one module. I _could_ see shared
dependencies being moved to dependencyManagement, but I don't like having mixed
criteria and further development overhead. No one is going to remember in the
future to promote an ad-hoc dependency to dependencyManagement if they
introduce a dependency that happens to be used elsewhere... So I rather keep it
as it is. It's already a huge step forward to have dependency versions in
properties, as it facilitates tracking and upgrade management big time.
---
I have pushed my changes to the branch. Have a look again and let me know what
you think.
Thanks for your constructive comments!
Regards,
Raúl.
> Basic OSGi support for Ignite
> -----------------------------
>
> Key: IGNITE-1270
> URL: https://issues.apache.org/jira/browse/IGNITE-1270
> Project: Ignite
> Issue Type: New Feature
> Components: general, osgi
> Reporter: Raúl Kripalani
> Assignee: Raúl Kripalani
> Fix For: 1.5
>
>
> Basics for a first iteration:
> * Manifest creation for all Java-based modules.
> * Apache Karaf features file to facilitate deployment (along with
> dependencies).
> * Make Ignite classloading OSGi-friendly and support different strategies.
> Future:
> * Support peer class-loading.
> * Potentially support remote 3rd dependency class-loading for Apache Karaf
> (via Pax Mvn URL and Aether, one can fetch a bundle from Maven repositories).
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
