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

Added OSGi dynamic import #377

Closed
wants to merge 1 commit into from
Closed

Added OSGi dynamic import #377

wants to merge 1 commit into from

Conversation

andsel
Copy link

@andsel andsel commented Sep 8, 2014

Added definition to OSGi descriptor to let the MapDB bundle load the classes from other bundles. This is necessary when a bundle uses loadClass and want to laod class definition exposed by other bundle, not included at compile time.

…er bundles (usually bundle that contains the beans serialized by MapDB)
@andsel andsel closed this Sep 14, 2014
@andsel andsel reopened this Sep 14, 2014
@niclash
Copy link

niclash commented Sep 15, 2014

This is a really poor way to support OSGi. The proper one is a better Serialization strategy in general, one that isn't as 'entrenched' in the MapDB's internals as the current one. I will actually vote against this change, if there is such a possibility... Sorry.

@andsel
Copy link
Author

andsel commented Sep 16, 2014

@niclash in general I could agree, but I don't understand a thing, how MapDB could introspect the client POJOs to serialize/desrialize them without loading the class definition? Perhaps a possible solution could be to use an external serializer and store data rough into the index, for example using ProtoBuf and pass the serialized form to MapDB, moving the resposability of serialize/deserialize into the client app?

@jankotek
Copy link
Owner

I know how OSGII support works in TopLink, it is complex to do it correctly. In mapdb jar I want option to have custom class loaders. And JAR should be correctly annotated. Rest should be in separate jar.

I will look at this in a few days before 1.1 is released

@niclash
Copy link

niclash commented Sep 16, 2014

We are also using MapDb in an OSGi project, and what we have done is roughly;
(Java 8)

public interface StorageService {
<R> R transact(BundleContext context, String password, Function<StorageTransaction, R> operations);
}

public interface StorageTransaction {
<T> ConcurrentMap<String, T> getHashMap(String name, Class<T> storedType);
}

So, we end up having client code do something like;

storage.transact( context, "secret", tx -> {
ConcurrentMap<String,Person> m = tx.getHashMap("persons", Person.class);
:
});

And in that transact() call we not only manages a MapDb transaction, but also establishes the Thread Context Classloader, which MapDb uses. And we do JacksonSerialization, but I think this approach would also work with the built-in Pojo serialization.

AND, we disallow cross-bundle access to databases, so each bundle has its own MapDB FileDB, which is encrypted. This also enforces good design.

@jankotek
Copy link
Owner

How about just providing your own class loader? Something like:

 DB db = DBMaker.newDB..().classLoader(yourOwn).make();

@niclash
Copy link

niclash commented Sep 16, 2014

That is good for the default serializer case, yes.

@andsel
Copy link
Author

andsel commented Sep 16, 2014

Hi Jan,
I'll try to put the client application bundle's classloader into MapDB
config, but is not much better than using dynamic import, because dynamic
import header is an osgi feature the other is like a trick to solve the
problem, IMHO.

@niclash
Copy link

niclash commented Sep 16, 2014

Andrea,
all due respect, but I think you don't understand OSGi properly, if you are promoting Dynamic-Import. It is in fact, the other way around; Dynamic-Import is an "last resort" solution, when no other ways are available. If you doubt my word, please contact Peter Kriens, Richard Hall or BJ Hargrave for authoritative opinion on this. OSGi Alliance Wiki says;

<quote>
Though DynamicImport-Package can be a life saver in certain circumstances it does revert the OSGi Framework to a very expensive class path for the packages involved. With DynamicImport-Package the OSGi Framework must revert to searching the public exported packages to find a match instead of the careful normal calculation. Especially using the wildcard is very harmful. The need for DynamicImport-Package usually is a symptom of a non-modular design.
</quote>

I would add; By not using Dynamic-Import, the bundles won't be started if packages are missing, but with Dynamic-Import, this guarantee doesn't exist. Also, framework end up struggling to do dynamic update/refresh when Dynamic-Import is involved, perhaps not a super-important feature in production, but very helpful during development.

If one is designing to be OSGi-friendly (honors to Jan for trying), the proper solution is for the client to either provide the classloader or provide a type which uses a classloader that can be used. If neither of those can be managed, for instance API compatibility must be maintained, then the TCCL is the option before Dynamic-Import.

@andsel
Copy link
Author

andsel commented Sep 16, 2014

Hi Niclas,
You are right,I don't have so much field experience wuth osgi,your
explanation is great,you convinced me,passing the classloader to be used to
load the class has it logic,so I ll try to fix on my code and close the
pull as not applicable.Thanks a lot for your complete explanation
Il giorno 16/set/2014 20:12, "Niclas Hedhman" notifications@github.com ha
scritto:

Andrea,
all due respect, but I think you don't understand OSGi properly, if you
are promoting Dynamic-Import. It is in fact, the other way around;
Dynamic-Import is an "last resort" solution, when no other ways are
available. If you doubt my word, please contact Peter Kriens, Richard Hall
or BJ Hargrave for authoritative opinion on this. OSGi Alliance Wiki says;

Though DynamicImport-Package can be a life saver in certain circumstances
it does revert the OSGi Framework to a very expensive class path for the
packages involved. With DynamicImport-Package the OSGi Framework must
revert to searching the public exported packages to find a match instead of
the careful normal calculation. Especially using the wildcard is very
harmful. The need for DynamicImport-Package usually is a symptom of a
non-modular design.

I would add; By not using Dynamic-Import, the bundles won't be started if
packages are missing, but with Dynamic-Import, this guarantee doesn't
exist. Also, framework end up struggling to do dynamic update/refresh when
Dynamic-Import is involved, perhaps not a super-important feature in
production, but very helpful during development.

If one is designing to be OSGi-friendly (honors to Jan for trying), the
proper solution is for the client to either provide the classloader or
provide a type which uses a classloader that can be used. If neither of
those can be managed, for instance API compatibility must be maintained,
then the TCCL is the option before Dynamic-Import.


Reply to this email directly or view it on GitHub
#377 (comment).

@andsel
Copy link
Author

andsel commented Sep 19, 2014

Hi Niclas and Jan,
I find that MapDB's issue #328 is similiar to this OSGi. So I would like a lot the setting of the class loader during DBMaker phase, there is a schedule when that feature is going to be available on MapDB?

@gravelld
Copy link

gravelld commented Apr 2, 2015

Agree the proper way of fixing this is adding support to specify a classloader.

However TCCL is not a workaround in many cases. @njbartlett wrote this up here: http://njbartlett.name/2010/08/30/osgi-readiness-loading-classes.html

Therefore I think in many cases we can only do a DynamicImport-Package: * right now.

In cases where you have control of the TCCL you can set it yourself before your call to MapDB.

I'd like to echo the props to @jankotek for adding support for OSGi thus far.

@jankotek
Copy link
Owner

jankotek commented Apr 5, 2015

@gravelld I can not reopen this issue. Could you please create a new bug for MapDB, with a few points what should be tested in 2.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants