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

GH-2043 make more imports optional of runtime-osgi #2045

Conversation

kenwenzel
Copy link
Contributor

GitHub issue resolved: #2043

Changed BND instructions to also make following imports optional:

  • org.locationtech
  • org.elasticsearch
  • ch.qos.logback

@kenwenzel kenwenzel force-pushed the issues/GH-2043_runtime_osgi_optional_imports branch from e81e1d0 to c6e18b4 Compare March 27, 2020 10:31
new optional imports are:
* org.locationtech
* org.elasticsearch
* ch.qos.logback

Signed-off-by: Ken Wenzel <kenwenzel@gmx.net>
@kenwenzel kenwenzel force-pushed the issues/GH-2043_runtime_osgi_optional_imports branch from c6e18b4 to 57ee6ef Compare March 27, 2020 10:51
@abrokenjester
Copy link
Contributor

abrokenjester commented Mar 27, 2020

I have no real way to judge this as I have little expertise on OSGi. @manuelfiorelli I know you've used the OSGi bundle in the past, can you take a quick look at this PR and tell us if it makes sense?

Open questions I have:

  1. is this change backwards-compatible for OSGi bundle users?
  2. will it require any changes at the user end for current consumers of the OSGi bundle?

@manuelfiorelli
Copy link
Contributor

Hi @jeenbroekstra. I looked at this PR without building the code by myself.
The first thing that I noticed is that the PR should actually do what it claims: just remove the 3 mentioned package imports. In fact, @kenwenzel also removed the the import of the packages org.eclipse.rdf4j.*, which are also exported by the bundle. This change should only affect users that deployed another bundle that exports these packages into the same container: in this case, the OSGi framework might force the rdf4j-runtime bundle to use the packages exported by the other bundle instead of its own versions.

In general, the main implication of removing a package import is the generation of exceptions when the JVM needs a class from that package. Since the JVM adopts a lazy loading strategy, these exceptions should be thrown only when the user tries to execute a portion of RDF4J that needs those classes:

  • in case of org.locationtech, the portion should include the GeoSpatial modules
  • in case of org.elasticsearch, the portion should be related to fulltext search
  • regarding ch.qos.logback, I don't know which portion of RDF4J explicitly uses that logger library: in most cases, RDF4J should just use slf4j, completely the choice of a logging library to the user.

Summarizing, the consequences of this PR are mainly two:

  • the user no longer needs bundles providing the packages org.locationtech and org.elasticsearch
  • but, if the user actually hits portions of the runtime that exploit these packages, the JVM will throw some exceptions.

In other words, this PR goes into the direction of having an RDF4J runtime which can be configured (by supplying specific dependencies) to enable a subset of the modules.

Existing users shouldn't be affected by this PR.

@abrokenjester
Copy link
Contributor

Excellent, thanks for your review @manuelfiorelli ! And @kenwenzel , thanks again for providing this patch!

@abrokenjester abrokenjester merged commit 54811bb into eclipse-rdf4j:master Mar 29, 2020
@kenwenzel kenwenzel deleted the issues/GH-2043_runtime_osgi_optional_imports branch November 10, 2021 15:56
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.

runtime-osgi: make more imports optional
3 participants