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

Network module SPI #440

Merged
merged 8 commits into from
Jun 15, 2018
Merged

Network module SPI #440

merged 8 commits into from
Jun 15, 2018

Conversation

mscheibler
Copy link

I like to enable the Paho Java Client to be extended by custom NetworkModules without introducing dependencies into the Paho Java client project.
So it will be possible to create my custom NetworkModule and still be able to use the Java Client from the Paho Project without the need to maintain a separate fork.

During the review i would really like to add some more documentation to the org.eclipse.paho.client.mqttv3.internal.NetworkModule interface. Especially about when / how often are the methods called. What are the consequences of the exceptions? How is the NetworkModule expected to behave on Exceptions, e.g. like who is triggering a reconnect?

Please make sure that the following boxes are checked before submitting your Pull Request, thank you!

  • You have signed the Eclipse ECA
  • All of your commits have been signed-off with the correct email address (The same one that you used to sign the CLA)
  • If This PR fixes an issue, that you reference the issue below. OR if this is a new issue that you are fixing straight away that you add some Description about the bug and how this will fix it.
  • If this is new functionality, You have added the appropriate Unit tests.

Maik Scheibler added 2 commits November 11, 2017 23:07
These files should be generated by the Eclipse Maven integration

Signed-off-by: Maik Scheibler <eclipse@scheibler-family.de>
The usage of the Java Service Provider Interface enables the deployment
of additional custom NetworkModules without modifications of the Paho
code.

Signed-off-by: Maik Scheibler <eclipse@scheibler-family.de>
Signed-off-by: Maik Scheibler <eclipse@scheibler-family.de>
@mscheibler
Copy link
Author

I spent some time to understand the way the Paho client uses its NetworkModules and wrote an APIdoc based on my findings. If i misunderstood something or missed an important part, please let me know.

@jpwsutton
Copy link
Member

Hi @mscheibler, Thank you for this contribution. It's definitely a nice way to refactor the older Network Module architecture and helpfully would resolve Enhancement #26 I think. The documentation and tests are a very welcome addition (We don't usually get those from contributors!). I'm going to pull your changes locally just to make sure that I fully understand how the SPI process works and once I'm happy I'll merge them in.

@jpwsutton jpwsutton self-requested a review November 16, 2017 11:26
@jpwsutton jpwsutton self-assigned this Nov 16, 2017
@mscheibler
Copy link
Author

Hi @jpwsutton
Thanks for the review. I've come to an issue yesterday evening with the Android packaging. The files under META-INF/services are currently not packed into the Android library and so the services are not known. This is not a direct problem of the Paho Java client, but must be considered when using in Android.
Because i want to use the library in Android i will definitely look into this issue, but this may take a day or two. I would suggest not to merge before this is solved. I keep you updated.

The explicit classloader is necessary for Android to be able to find and
load the declared NetworkModuleFactories

Signed-off-by: Maik Scheibler <eclipse@scheibler-family.de>
@mscheibler
Copy link
Author

Hi @jpwsutton
I did a small change for Android, so the ServiceLoader is able to find the declared Services under Android. The deployment problem did not really exist, i made a mistake in my own Android project. (transient dependencies are not included in the APK)
I was able to test my refactoring with the org.eclipse.paho.android.sample app. Unfortunately the unit-tests are currently stuck in the reconnect test, but this appears not to be related to my changes. I think the pull request can be merged if you see fit.
Best regards Maik

@mscheibler
Copy link
Author

Hello @jpwsutton
Did you have a chance to review the pull request? We are already using a patched version without any problems.
Best regards Maik

Maik Scheibler added 3 commits January 25, 2018 16:59
Conflicts:
	org.eclipse.paho.client.mqttv3/src/main/java/org/eclipse/paho/client/mqttv3/MqttConnectOptions.java

Signed-off-by: Maik Scheibler <eclipse@scheibler-family.de>
Signed-off-by: Maik Scheibler <eclipse@scheibler-family.de>
Conflicts:
	org.eclipse.paho.client.mqttv3.test/src/test/java/org/eclipse/paho/client/mqttv3/test/MqttConnectOptionsTest.java
	org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/MqttAsyncClient.java
@mscheibler
Copy link
Author

Hello @jpwsutton
Is there anything i can do to help the pull request to get merged? I updated it again to resolve the conflicts.

@jpwsutton jpwsutton added this to the 1.3.0 milestone Jun 15, 2018
@jpwsutton jpwsutton merged commit 6d2906e into eclipse:develop Jun 15, 2018
@jpwsutton
Copy link
Member

Hi @mscheibler, Sorry for the delay in getting this merged, I've finally had some time to sit down and go through it again, and am very glad to have merged it today. Thanks for all of your hard work!

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

Successfully merging this pull request may close these issues.

None yet

4 participants