apache / pekko

Build highly concurrent, distributed, and resilient message-driven applications using Java/Scala
https://pekko.apache.org/
Apache License 2.0
1.22k stars 149 forks source link

Use sbt's multi-jvm instead of our manual implementation? #548

Open mdedetrich opened 1 year ago

mdedetrich commented 1 year ago

As a result of the discussion at https://github.com/apache/incubator-pekko-http/pull/297#issuecomment-1669011499 I just found out that there is already a sbt-multi-jvm plugin that appears to be identical to ours and was originally upstream by Akka/Akka community, presumably for Akka to eventually remove the multi jvm setup in their builds which we inherited.

I think we should seriously consider upstreaming any necessary changes and then using the plugin, especially considering that we now have duplicated code between pekko and pekko-http.

@jrudolph @pjfanning @He-Pin @raboof wdyt ?

mdedetrich commented 1 year ago

From https://github.com/apache/incubator-pekko-http/pull/297#issuecomment-1669033563 @He-Pin

We need wait the Controller transited to connectable before start the Player.

I think this concept is easily generalizable given that its not a problem thats specific to Pekko (after testing clusters is the reason we are using multiple JVM's in the first place)

Need submit a PR to sbt-multi-jvm and waiting a new snapshot, which may take sometimes and is not a quick fix.

This is a non issue at least in regards to using sbt-multi-jvm right now. I am thinking about this long term, but typically if its seen there are significant contributions made to a project than you get added as a maintainer (these are community projects after all).

I did not diff the code with sbt-multi-jvm line by line, but there must be some reason for Akka was keep it separately

From what I could see by quickly glancing at the code, the intention was to replace sbt-multi-jvm withe code in Akka but they never got around to it likely because of priority/effort required.

He-Pin commented 1 year ago
  1. That's was a quick fix so I duplicated the code to keep night build works.
  2. For future, we should extract all shared plugins to something as what you once suggested and typelevel does, eg: apache-sbt-pekko?
mdedetrich commented 1 year ago

That's was a quick fix so I duplicated the code to keep night build runing.

Understood and fine for now

For future, we should extract all shared plugins to something as what you once suggested and typelevel does, eg: apache-sbt-pekko?

This makes sense for features that are unique to pekko, i.e. think our Paradox theme or setting up the organization of our projects to org.apache.pekko. multi-jvm however is just a general feature so it makes more sense if its possible to use sbt-multi-jvm.

This isn't any different to sbt-license-report, which our Pekko projects use but is an sbt community plugin (see https://github.com/sbt/sbt-license-report) and if you look at https://github.com/sbt/sbt-license-report/pulls?q=is%3Apr+author%3Amdedetrich and https://github.com/sbt/sbt-license-report/releases you can see that contributing upsream and getting a release wasn't an issue at all.

As I said before, generally speaking if its shown that you contribute features upstream there isn't usually a problem in those releases being made.

He-Pin commented 1 year ago

The sbt-multi-jvm can go upstream and you saw there are so many backlog there too.

mdedetrich commented 1 year ago

The sbt-multi-jvm can go upstream and you saw there are so many backlog there too.

If you mean by issues then yes there does seem to be quite a few open https://github.com/sbt/sbt-multi-jvm/issues but there aren't any PR's for it. And with issues such as https://github.com/sbt/sbt-multi-jvm/issues/36 this just strengthens my earlier assumption which is that Akka was upstreaming the sbt-multi-jvm functionality but didn't get around to finishing it.

I think that if we start opening up PR's to actually solve the issues mentioned before then that should get things going.

He-Pin commented 1 year ago

I think I can submit a PR which do not need the upstream change, @mdedetrich , which will do another round of refractory to the current Player impl. @mdedetrich .