lagom / lagom-akka-discovery-service-locator

Apache License 2.0
5 stars 7 forks source link

[WIP] Cross build to Lagom 1.4.x and 1.5.x #80

Closed octonato closed 5 years ago

octonato commented 5 years ago

Fixes https://github.com/lagom/lagom-akka-discovery-service-locator/issues/79

TODO: we need some kind of integration tests. As of today, its only testing compilation.

octonato commented 5 years ago

It was a good idea to add tests and check again 1.4.x.

I have found a few issues:

Lagom 1.4.11 uses Akka 2.5.20, but this library will bump it to 2.5.22, but not all akka deps are bumped to 2.5.22. At runtime (in tests) we get an error:

java.lang.NoClassDefFoundError: akka/util/ccompat/imm/package$SortedSetOps$
    at akka.cluster.ClusterEvent$.diffMemberEvents(ClusterEvent.scala:455)
    at akka.cluster.ClusterDomainEventPublisher.publishDiff(ClusterEvent.scala:610)
    at akka.cluster.ClusterDomainEventPublisher.publishChanges(ClusterEvent.scala:606)
    at akka.cluster.ClusterDomainEventPublisher$$anonfun$receive$1.applyOrElse(ClusterEvent.scala:547)
    at akka.actor.Actor.aroundReceive(Actor.scala:539)
    at akka.actor.Actor.aroundReceive$(Actor.scala:537)
    at akka.cluster.ClusterDomainEventPublisher.aroundReceive(ClusterEvent.scala:522)
    at akka.actor.ActorCell.receiveMessage(ActorCell.scala:610)
    at akka.actor.ActorCell.invoke(ActorCell.scala:579)
    at akka.dispatch.Mailbox.processMailbox(Mailbox.scala:268)
    at akka.dispatch.Mailbox.run(Mailbox.scala:229)
    at akka.dispatch.Mailbox.exec(Mailbox.scala:241)
    at akka.dispatch.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
    at akka.dispatch.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
    at akka.dispatch.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
    at akka.dispatch.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)
Caused by: java.lang.ClassNotFoundException: akka.util.ccompat.imm.package$SortedSetOps$
    at java.net.URLClassLoader.findClass(URLClassLoader.java:381)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357)

I tried to configure the Akka version via settings but that won't solve the problem because ultimately the jar we produce here will need to bring in 2.5.22 anyway.

Moreover, Akka < 2.5.22 are not the version that we want anyway. We make use of SRV Lookup and the parser before 2.5.22 was not correct.

Previously we were using an internal, fixed, version of that same parser, but that was removed because we moved to 2.5.22.

The released version (v1.0.0) does still have the internal parser and it should work with Lagom 1.4.x, but we still have the problems of akka version alignment. Lagom 1.4 is on 2.5.20, Lagom 1.5 is on 2.5.22 and this Akka Service Locator 1.0 is on 2.5.21.

Because Lagom doesn't bring in Akka Service Discovery (only in dev-mode), we will always have a misalignment.

Instead of starting a matrix of which Lagom version should be use with which version of Akka ServiceLocator, I prefer to update Lagom 1.4.x to Akka 2.5.22 and have only one version of Akka ServiceLocator.

That's for me the option with less overhead and more value.

octonato commented 5 years ago

And this exposes another issue, because Lagom doesn't depend on Akka Service Discovery, each time we bump the Akka version in Lagom we will need to bump it here and advice users to update both together.

ihostage commented 5 years ago

And this exposes another issue, because Lagom doesn't depend on Akka Service Discovery, each time we bump the Akka version in Lagom we will need to bump it here and advice users to update both together.

Sounds like a potential source of many questions on the forum and Gitter channel. 😄 Perhaps it makes sense to make it a Lagom module for sync a version of Akka dependencies. 🤔 Otherwise, necessarily need a dependency matrix of Lagom <-> Service Locator. ☝️

octonato commented 5 years ago

Sounds like a potential source of many questions on the forum and Gitter channel. 😄

Exactly, the less of this the better for us all.

Perhaps it makes sense to make it a Lagom module for sync a version of Akka dependencies. 🤔

Maybe not a solution for 1.4.x, but may, for Lagom 1.5.x we can add the dependency on akka service discovery. Assuming that most of users using Akka ServiceLocator will be Lagom 1.5.x users.

Otherwise, necessarily need a dependency matrix of Lagom <-> Service Locator. ☝️

Correct and that's what I would like to avoid. Worst case scenario we will need to maintain a matrix, but that still generates questions and issues because usually people only read the matrix after hitting the issue and asking around.

dwijnand commented 5 years ago

Let's just merge this into the Lagom repo. It's no longer experimental, and we can have minimally different versions for 1.4.x and 1.5.x without having to maintain (and communicate) more version matrices.

octonato commented 5 years ago

Yeah, maybe that's a what we should do.

I'm just wondering how much work we will be adding just because of it.

The move means:

We will need to move the code and add docs. Then adapt the guides and shopping cart. I don't expect issues in back-porting it except for the build in 1.4.x. That's will probably won't cherry-pick cleanly, but it's a minor issue.

The status-quo means:

Release Lagom 1.4.12 (PR is merged already), add a notice on the guide that is only works with 1.4.12 or later. Then, new releases will come and each time we will either have to release 1.4.x and 1.5.x or start to maintain a matrix.

As soon as we update Akka in Lagom 1.5.x and here, we will need to tell people: "watch out, when using with 1.4.x you need that previous version of Akka ServiceLocator"

Alternative path:

Lock Akka version here (v2.5.22). This is a good thing in general because we don't want that this library update the Akka versions where it is used.

We add both in Lagom 1.4.x and 1.5.x the dependency to Akka Service Discovery (it is there already, but only on dev mode).

As such, whenever we update the Akka version in Lagom, it will evict whatever version is being used here.

That to say, we really need a good solution to align the Akka version in Lagom.

dwijnand commented 5 years ago

I'm just wondering how much work we will be adding just because of it.

Yeah, good to analyse it.

The move means:

We will need to move the code and add docs. Then adapt the guides and shopping cart. I don't expect issues in back-porting it except for the build in 1.4.x. That's will probably won't cherry-pick cleanly, but it's a minor issue.

Add docs? Why does moving the code require adding docs? Is it not also just move the current docs into the lagom website?

And adapting the guides and shopping cart apps just means update it to use the Lagom version, right? (If so then it's no different to another of the other strategies: a new release can require updating version references.)

octonato commented 5 years ago

Add docs? Why does moving the code require adding docs? Is it not also just move the current docs into the lagom website?

If it becomes part of Lagom it needs documentation in Lagom. Part of the README file here can be used, but some re-wording is needed. That library can be part of the Lagom repo, but that must be something that users need to explicitly opt-in and configure. If we enable it by default we break existing applications.

And adapting the guides and shopping cart apps just means update it to use the Lagom version, right?

Not completely true because of the above. I guide still needs to explain it, but the adaptations are minimal

dwijnand commented 5 years ago

I never meant by default. I assume it would be an opt-in module of Lagom like already existing ones.