[
https://issues.apache.org/jira/browse/IGNITE-1270?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15026611#comment-15026611
]
Romain Gilles edited comment on IGNITE-1270 at 11/25/15 11:13 AM:
------------------------------------------------------------------
Hi,
First of all I like the idea of playing with the classloader. Damn it, why I
didn't think about it before ;). Your approach doesn't touch the serialization
mechanism and it is interesting but I think it may leads to some unsolvable use
cases to resolve the class. But maybe this is not the purpose of this ticket.
I had a look to the branch. I have some questions about it.
Why you introduce the constrain of having only one ignite instance per jvm?
h4. IgniteOsgiUtils
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.
h6. gridCount()
I think this method name is misleading. For me what you are counting this the
number of ignite instance and not the number of grid. I'm not sure but I will
more incline to use the node term instead of the grid one.
h6. classloaders()
This method gives access to the client to the internal map. Therefore if I want
to publish more than one ignite instance I just have to call
IgniteOsgiUtils.classloaders().clear() in my extension of
IgniteOsgiContextActivator.
h4. BundleDelegatingClassLoader
Why don't you use the parent child classloader delegation?
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.
h4. ContainerSweepClassLoader
This is cool.
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...
You should also take care of the bundle state before requesting the class
loading as only at least RESOLVED bundles have a class loader.
There is also a problem with fragment with an exception maybe you can skip
fragments.
If you call loadClass on an INSTALLED it will attempt to resolve and I'm not
sure this is what the end user want.
If you call it on un UNSTALLED then exception.
This may impact the sweepContainer(..) method and the {{return
resolved.get(name).loadClass(name);}} block of code.
h4. IgniteOsgiContextActivator
Maybe the Abstract key word should be used in the name to make it crystal clear
:)
You should make the {{start(BundleContext context)}} and {{stop(BundleContext
context)}} final in order to avoid the hack I mention above.
h4. Maven
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 hope it will help. Thank for this huge work. More will come :)
Romain.
was (Author: rogilles):
Hi,
First of all I like the idea of playing with the classloader. Damn it, why I
didn't think about it before ;). Your approach doesn't touch the serialization
mechanism and it is interesting but I think it may leads to some unsolvable use
cases to resolve the class. But maybe this is not the purpose of this ticket.
I had a look to the branch. I have some questions about it.
Why you introduce the constrain of having only one ignite instance per jvm?
h4. IgniteOsgiUtils
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.
h6. gridCount()
I think this method name is misleading. For me what you are counting this the
number of ignite instance and not the number of grid. I'm not sure but I will
more incline to use the node term instead of the gird one.
h6. classloaders()
This method give access to the client to the internal map. Therefore if I want
to publish more than one ignite instance I just have to call
IgniteOsgiUtils.classloaders().clear() in my extension of
IgniteOsgiContextActivator.
h4. BundleDelegatingClassLoader
Why don't you use the parent child classloader delegation?
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.
h4. ContainerSweepClassLoader
This is cool.
Do you plan to support the dynamic addition and removal of bundles? This
dynamicity may impact the nonResolvable Set. As some classes may become
resolvable.
You should also take care of the bundle state before requesting the class
loading as only at least RESOLVED bundle have a class loader.
There is also a problem with fragment with an exception maybe you can skip
fragments.
If you call loadClass on an INSTALLED it will attempt to resolve and I'm not
sure this is what the end user want.
If you call it on un UNSTALLED then exception.
This may impact the sweepContainer(..) method and the {{return
resolved.get(name).loadClass(name);}} block of code.
h4. IgniteOsgiContextActivator
Maybe the Abstract key word should be used in the name to make it crystal clear
:)
You should make the {{start(BundleContext context)}} and {{stop(BundleContext
context)}} final in order to avoid the hack I mention above.
I hope it will help.
Romain.
> 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)
