pekko-grpc icon indicating copy to clipboard operation
pekko-grpc copied to clipboard

use Pekko JavaConverter

Open pjfanning opened this issue 1 year ago • 5 comments

  • just updated the 'runtime' module
  • other modules don't necessarily have a dependency on pekko-actor module so can't use Pekko JavaConverter unless we add a dependency - I'd prefer not to do this

pjfanning avatar Nov 03 '24 12:11 pjfanning

IIRC think the only reason org.apache.pekko.util.ccompat existed was because back then scala-collection-compat did not yet promise binary compatibility, so we didn't dare to add it as a dependency, but did want the functionality (https://github.com/apache/pekko/blob/main/actor/src/main/scala-2.13%2B/org/apache/pekko/util/ccompat/package.scala#L27-L29). Now that scala-collection-compat has stabilized, wouldn't it be better to use scala-collection-compat instead of org.apache.pekko.util.ccompat? Perhaps we should even do that in Pekko...

raboof avatar Nov 03 '24 15:11 raboof

There is also the issue of having the jar dependency. I don't think we can add a jar dependency like scala-collection-compat in a patch release but we could do it in a minor release.

The plus side to this PR is to avoid using deprecated classes in scala-library jar.

pjfanning avatar Nov 03 '24 16:11 pjfanning

There is also the issue of having the jar dependency. I don't think we can add a jar dependency like scala-collection-compat in a patch release but we could do it in a minor release.

I'm not sure we can't do it in a patch release but I have no problems having the next release be a minor release. I think I'd prefer adding a dependency on scala-collection-compat over adding a dependency on Pekko code marked 'internal API' - ideally we should be able to eventually replace it there as well.

raboof avatar Nov 03 '24 16:11 raboof

IIRC think the only reason org.apache.pekko.util.ccompat existed was because back then scala-collection-compat did not yet promise binary compatibility, so we didn't dare to add it as a dependency, but did want the functionality (https://github.com/apache/pekko/blob/main/actor/src/main/scala-2.13%2B/org/apache/pekko/util/ccompat/package.scala#L27-L29). Now that scala-collection-compat has stabilized, wouldn't it be better to use scala-collection-compat instead of org.apache.pekko.util.ccompat? Perhaps we should even do that in Pekko...

Another thing that is going on is the inliner is taking advantage of the source being manually there and hence its inlining the differences away, albeit I don't know whether this is actually creating a performance improvement or not as JDK might be inlining all hotspots anyways.

There are also a couple of cases of some subtles differences to scala-collection-compat but I can't remember it on the top of my head, I can search for them if requested.

Obviously if we want to be ultra safe it makes sense to leave it as is, at least until 2.12 is dropped. Slick is also manually including the source for 2.12 for its own reasons.

mdedetrich avatar Nov 03 '24 19:11 mdedetrich

I'm neutral about this PR. We have used the Pekko JavaConverter in most of our modules and this is probably one of the few exceptions. I would prefer to use to the existing deprecated Scala JavaConverter than to introduce a dependency on scala-collection-compat.

pjfanning avatar Nov 03 '24 19:11 pjfanning