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.

Sign up for GitHub

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

Jump to bottom

Better OSGi support #269

Merged
merged 2 commits into from
Feb 13, 2018
Merged

Better OSGi support #269

merged 2 commits into from
Feb 13, 2018

Conversation

lburgazzoli
Copy link
Collaborator

This PR is aimed to improve JEtcd OSGi support and includes the following changes to the current projects:

  • there is a new artifact jetcd-common to have better separation of modules
  • there is a new artifact jetcd-osgi that provides Apache Karaf features and some JEtcd services exposed via OSGi DS
  • jetcd-all is now enabled and shades all the dependencies jetcd requires. this is needed because grpc has some split packages issues so it is not possible to deploy "plain" grpc artifacts in OSGi environments

Some more work may be needed but I'd like to get this merged so people can test it.

@fanminshi
Copy link
Member

@lburgazzoli i took a quick glance of the code. it seems to me there are lot of code changes in different areas

  • there is a new artifact jetcd-common to have better separation of modules
  • there is a new artifact jetcd-osgi that provides Apache Karaf features and some JEtcd services exposed via OSGi DS
  • jetcd-all is now enabled and shades all the dependencies jetcd requires. this is needed because grpc has some split packages issues so it is not possible to deploy "plain" grpc artifacts in OSGi environments

is it possible to submit separate standalone pr for each? in that way, it can help better to understand all the changes happening in the pr.

@lburgazzoli
Copy link
Collaborator Author

@fanminshi I can try to split it into different PRs but not sure if I can make it soon.

Most of the code changes are just code relocation:

  • jetcd-common at the moment contains only the etcd exceptions relocated from jetcd-core/com.coreos.jetcd.exception
  • jetcd-resolver is the new home for jetcd/com.coreos.jetcd.resolver
  • jetcd-resolver-dbs-srv is almost a rename of jetcd-discovery-dns-srv
  • jetcd-osgi is a brand new module with 2 small classes and related tests

The main changes apply to the POMs to properly configure OSGi stuffs and ion particular to jetcd-all which is now used as OSGi bundle as workaround for the following grpg-java issue with module systems:

fanminshi
fanminshi reviewed Jan 8, 2018
View reviewed changes
etc/scripts/run_etcd_docker.sh
@@ -87,7 +87,7 @@ docker run \
--name etcd-ssl \
--hostname etcd-ssl \
--network etcd \
--volume $CERT_HOME:/etc/ssl/etcd \
--volume $CERT_HOME:/etc/ssl/etcd:Z \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's Z option for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Z is for SElinux, it should jave no effects on systems were SELinux is disabled

jetcd-all/src/main/java/com/coreos/jetcd/all/package-info.java
* limitations under the License.
*/

package com.coreos.jetcd.all;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line?

jetcd-osgi/src/main/resources/features.xml

<feature prerequisite="true">wrap</feature>

<!-- google -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this commented out? prefer codebase to be as minimum as possible.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is left as documentation to show how the karaf feature should looks like once grpc solves the split package issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see. Could you add comment to explain the intention?

jetcd-resolver-dns-srv/src/main/java/com/coreos/jetcd/resolver/dnssrv/DnsSrvUriResolver.java
@@ -50,7 +48,7 @@
ENV.put("java.naming.provider.url", "dns:");
}

private ConcurrentMap<String, EquivalentAddressGroup> cache;
private ConcurrentMap<String, SocketAddress> cache;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why changing EquivalentAddressGroup to SocketAddress?

Copy link
Collaborator Author

@lburgazzoli lburgazzoli Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reduce packages inter dependencies among OSGi bundles

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it.

jetcd-resolver/src/test/java/com/coreos/jetcd/resolver/NameResolverTest.java
@@ -31,7 +29,7 @@ public void testDefaults() throws Exception {
final URIResolverLoader loader = URIResolverLoader.defaultLoader();
final SmartNameResolver resolver = new SmartNameResolver("etcd", Collections.emptyList(), loader);

assertThat(resolver.getResolvers().stream().anyMatch(DirectUriResolver.class::isInstance)).isTrue();
Assertions.assertThat(resolver.getResolvers().stream().anyMatch(DirectUriResolver.class::isInstance)).isTrue();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason on changing imports to import org.assertj.core.api.Assertions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No done intentionally, guess the IDE did it while refactoring. Will fix it

@fanminshi
Copy link
Member

@lburgazzoli looks good in general. I quickly studied osgi and gain an general idea on your osgi implementation. it would be better if someone with more expertise can also review the osgi part of code.

also the ci is broken. could you check upon that.

@lburgazzoli
Copy link
Collaborator Author

@fanminshi I'm having some issues ot understand why the OSGi tests fails on travis-ci but not locally and I do not have time to digg into further so would it be ok if I mark as @ignored OSGi tests for the moment ? I'd like to get this PR merged asap so it does not diverge too much from master.

@fanminshi
Copy link
Member

@lburgazzoli I understand your concern. I usually don't want to merge anything that has potential bug whether it is ci or code itself. Is there a rush in pushing osgi support in? afaik, not many changes are being pushed into jetcd. I think you are safe from worrying about pr diverging. Could we get another week or two of debugging. I might take a look as well. If we can't figure out by then, we can the pr with @ignored test option.

@lburgazzoli
Copy link
Collaborator Author

Yeah I have no issue to wait some more weeks. In fact my main goal is to get jetcd in apache-camel and OSGi support is definitively something that would be more that welcome.

@lburgazzoli
Copy link
Collaborator Author

@fanminshi I disabled the test because I have very little time at the moment to digg into it and it should be an issue with temporary directories or ports as there's no other reason to have such a different behavior, I 'll have a further look as soon as possible but it would be nice if we can get this merged.

@lburgazzoli
Copy link
Collaborator Author

@fanminshi I've rebased against latest master, can we get this merged ?

@fanminshi
Copy link
Member

yeah, let's get this merged.

@fanminshi fanminshi merged commit 71c0d01 into etcd-io:master Feb 13, 2018
@lburgazzoli lburgazzoli deleted the osgi branch February 13, 2018 23:14
@tisonkun
Copy link
Contributor

tisonkun commented Aug 8, 2019

I found several diffs on changing import io.etcd.jetcd.Auth; to import io.etcd.jetcd.api.Auth;(e.g., io.etcd.jetcd.auth.*Response). However, the former is the proper one. Maybe it is done by automatically refactoring.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fanminshi fanminshi fanminshi left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lburgazzoli @fanminshi @tisonkun