Make flatMapPrefix javadsl using java.util.List #271

He-Pin · 2023-03-24T17:14:45Z

This will make it easier to work with in Java. And the scaladsl was seq

pjfanning · 2023-03-24T18:03:20Z

A 1.1.0 change for me and we should continue to include the Old API and deprecate it.

He-Pin · 2023-03-24T18:12:31Z

I think it's source compatible(if lambda is being used), and can't have both because they are the same after being erased.

mdedetrich · 2023-06-17T12:03:06Z

@He-Pin @pjfanning So I just noticed this and its kind of annoying because if we do need to do this then it kind of should be done now but I would like more context as to why this is problematic in Java.

@He-Pin What exactly is the issue here from Java's side in using Iterable? I mean java.util.List might be easier but its also probably too strict of a type and there are other types of collections that aren't List which you would want to use this function with.

Maybe this calls for adding some helper functions to org.apache.pekko.util.Helpers?

pjfanning · 2023-06-17T12:07:16Z

I think it is too late to do this in 1.0.0. I really thought we were ready for a 1.0.0 RC and to sneak in changes like this and force us to go through even more testing.

I am not too sure we need the change at all - Iterable is an ok return type - I don't see the major benefit and then we have the cost of the API disruption.

mdedetrich · 2023-06-17T12:09:46Z

Yeah I want to avoid sneaking this in as well unless it really has ramifications further down the line which is why I want context as to what problem we are solving. If by "easer to work in Java" we just mean saving some lines of code when converting from java.util.List to java.lang.Iterable within Java then such a solution creates more problems for the reasons stated earlier.

He-Pin · 2023-06-17T15:43:41Z

@mdedetrich With List type, it can gives you more methods to be accessible.

i come up it with real production code which i have to do a cast instead.

And as this is not a binary compatible change after release of 1.0.0

He-Pin · 2023-06-17T15:54:30Z

And this is another reason.

pjfanning · 2023-06-17T15:57:57Z

I am very much against a 1.0.0 change. I don't think this change justifies delaying the release. It is an API change and I think the majority of Pekko community users just want an Akka like release.

He-Pin · 2023-06-17T16:03:26Z

I have to say it's a mistake when it's Java.lang.Iterable, it should be Java.util.List instead as what we said in the contribution guide.

And if it's not landing in 1.0.0, it may never have a chance anymore.

He-Pin · 2023-06-17T16:06:56Z

@mdedetrich It instead liberating the usage on Java side.

And prefixAndTail is using java.util.List too, just as the contribution guide.

pjfanning · 2023-06-17T16:07:44Z

I have to say it's a mistake when it's Java.lang.Iterable, it should be Java.util.List instead as what we said in the contribution guide.

And if it's not landing in 1.0.0, it may never have a chance anymore.

We've had months to discuss. Why choose the absolute last minute before the release to discuss this?

Akka users have used the existing API for years. I do not understand why this is so important to change in Pekko.

The Pekko community seemed to have an agreement to try to get an Akka like release done and to follow up with bigger changes in 1.1.0.

We really have to release soon. The Akka CVEs that they are refusing to open source means that there are Akka users who want a quick Pekko release so that they can use a CVE-free alternative.

He-Pin · 2023-06-17T16:13:57Z

This change will not affect who using lamda, but those who using Function Constant.

If it has to take long time to discuss, i think we can defer it to next major version, but it still a inherented mistake with using Java.lang.Iterable.

pjfanning · 2023-06-17T16:15:24Z

This change will not affect who using lamda, but those who using Function Constant.

If it has to take long time to discuss, i think we can defer it to next major version, but it still a inherented mistake with using Java.lang.Iterable.

I would support a 1.1.0 change. There are other places in the code that use Function of Iterable and they should be considered to be changed too - I found 6 and this PR just changes 2.

He-Pin · 2023-06-17T16:17:33Z

@pjfanning PLS reschedule it, i'm away from computer.

pjfanning · 2023-06-17T16:18:49Z

mdedetrich · 2023-06-17T16:42:11Z

If we do breaking API changes that would need to be 2.0.0 according to semver. The easiest option would be to come up with a different method name and deprecate the older one.

mdedetrich · 2023-06-18T11:11:54Z

@pjfanning On this point, should I make a thread on the mailing list talking about semver? I did PR's in the projects setting the versioning scheme of the projects to semver but it sounds a lot of people are not aware of this (either by what semver means or the fact that we follow semver at all).

pjfanning · 2023-06-18T11:39:10Z

@mdedetrich I'd like to keep the mailing threads focused on the release - semver can be discussed at any time.

I would be shocked if many people don't expect us to follow semver principles.

I think this issue can be fixed at any time by adding new methods and not trying to change the existing ones. A new method could be called flatMapPrefixWithList instead of flatMapPrefix (or some other name, this is just a suggestion).

mdedetrich · 2023-06-18T12:27:04Z

@mdedetrich I'd like to keep the mailing threads focused on the release - semver can be discussed at any time.

@pjfanning The thing is, semver is very black and white since we have https://github.com/apache/incubator-pekko/blob/main/build.sbt#L17. Thats why I am bringing this up, if you have ThisBuild / versionScheme := Some(VersionScheme.SemVerSpec) set in sbt, you MUST follow semver because that setting is literally instructs sbt to refuse resolving dependencies that break semver (i.e. you resolve pekko 1.1 and pekko 2.1 in the same project).

The reason why I set that is that Akka follows semver which Pekko was meant to follow and this setting makes it official.

I think this issue can be fixed at any time by adding new methods and not trying to change the existing ones. A new method could be called flatMapPrefixWithList instead of flatMapPrefix (or some other name, this is just a suggestion).

If we decide to do this then its fine (flatMapPrepend is probably a better alternative name), but if we instead decide to change the existing method then we either have to frontload these change/s before release OR remove ThisBuild / versionScheme := Some(VersionScheme.SemVerSpec) and don't claim we follow semver. On that note, not following semver is a serious decision because you are basically stating you can potentially break the Pekko ecosystem at any time. While having a Pekko 2.x at some point (which is what allows for a breaking change) is probably going to happen at some time, it should not be done lightly.

There are other points that you raised like pekko-connectors, i.e. there is an argument that pekko-connectors probably shouldn't follow semver because of how frequently they change and more importantly how historically frequent those changes have technically broken ABI

pjfanning · 2023-06-18T12:42:48Z

pekko-connectors has docs stating that it is subject to change.

pekko core libs have been stable for a long time. We should enable the mima filter checks soon after the 1.0.0 release. Any time we have to add an exception to the mima checks is a very very sad day and should be signed off by the community. And the next release number should be controlled by those binary compatible changes. Any major breakages mean that we need a major release.

mdedetrich · 2023-06-18T12:47:04Z

pekko-connectors has docs stating that it is subject to change.

In that case ThisBuild / versionScheme := Some(VersionScheme.SemVerSpec) should be removed from pekko-connectors, is there any other module that documents this?

pekko core libs have been stable for a long time. We should enable the mima filter checks soon after the 1.0.0 release.

That means we are going to follow semver.

Any time we have to add an exception to the mima checks is a very very sad day and should be signed off by the community. And the next release number should be controlled by those binary compatible changes. Any major breakages mean that we need a major release.

In that case we either change the function now or add another method name, the latter being much more appealing since it also provides an upgrade path.

Regarding new method name, I think that flatMapTakeAndApply` is a good candidate, and is arguably better than the current name in describing what the function actually does.

pjfanning · 2023-06-18T13:09:05Z

I'm begging - can we stop discussing adding a change for this now? We can add a change later - and one that doesn't break binary or source compatibility.

The community wants a release.

This is not a critical issue. I have never come across a scenario where minor issues are turned into release blockers at the very last minute before a release.

We can do a v1.0.1 with an additional API in a few months.

mdedetrich · 2023-06-18T13:19:06Z

I'm begging - can we stop discussing adding a change for this now? We can add a change later - and one that doesn't break binary or source compatibility.

I never said we should add the change now. I am saying we should be clear about semver, that's it. And the discussion was needed because adding semver to pekko-connectors was a mistake, one that could have been easily forgotten and create a lot of harm/chaos to users.

It was suggested earlier to add a breaking change in 1.1.0, I am saying we should not do that.

I came up with alternate names if @He-Pin wants to update the PR with an added method and if he follows that course of action its obviously not a priority, it can be done whenever.

pjfanning · 2023-06-18T13:36:48Z

With semver, we may want a mail thread but can we exclude pekko and pekko-http? I think it is fair to say that those 2 libs are very stable and the whole community expects strict semver. These projects are also used by many downstream libs so binary incompatible changes will have big negative impacts.

There are questions about the rest of the Pekko repos. You could argue that we impose strict semver now and ramp up version numbers if we have to make binary incompatible changes in future releases. Or we say that 1 or 2 of the repos are still unstable and we can't impose strict semver on them. All in all, binary incompatible changes outside of pekko and pekko-http will have more limited impacts that those for pekko and pekko-http.

mdedetrich · 2023-06-18T13:42:38Z

With semver, we may want a mail thread but can we exclude pekko and pekko-http? I think it is fair to say that those 2 libs are very stable and the whole community expects strict semver. These projects are also used by many downstream libs so binary incompatible changes will have big negative impacts.

I agree with that, possible pekko-grpc ontop of that as well

There are questions about the rest of the Pekko repos. You could argue that we impose strict semver now and ramp up version numbers if we have to make binary incompatible changes in future releases. Or we say that 1 or 2 of the repos are still unstable and we can't impose strict semver on them. All in all, binary incompatible changes outside of pekko and pekko-http will have more limited impacts that those for pekko and pekko-http.

I am making a PR to remove semver from pekko-connectors because after skimming through the history, its kind of delusional for it to ever realistically follow semver due to the fact that it supports so many artifacts (via different connectors) inside a single mono repo with a single version (if it did follow semver it would break the major so often that the semver becomes pointless). There is an argument to be made here whether we should gradually split out the connectors into their own versions/repos, but not one that should be had now especially with the amount overhead separate git repos create. As you pointed out, we have bigger fish to fry (i.e. a release).

With the other repos we can evaluate on a case by case basis, a lot of them aside of connectors seem quite stable so the risk is lower but it should be looked into properly.

He-Pin · 2024-01-05T20:34:21Z

I think this change should be binary compatible too, as the function is erased to function[any,any]

  // access flags 0x1
  // signature <Out2:Ljava/lang/Object;Mat2:Ljava/lang/Object;>(ILorg/apache/pekko/japi/function/Function<Ljava/lang/Iterable<TOut;>;Lorg/apache/pekko/stream/javadsl/Flow<TOut;TOut2;TMat2;>;>;)Lorg/apache/pekko/stream/javadsl/Flow<TIn;TOut2;TMat;>;
  // declaration: org.apache.pekko.stream.javadsl.Flow<In, Out2, Mat> flatMapPrefix<Out2, Mat2>(int, org.apache.pekko.japi.function.Function<java.lang.Iterable<Out>, org.apache.pekko.stream.javadsl.Flow<Out, Out2, Mat2>>)
  public flatMapPrefix(ILorg/apache/pekko/japi/function/Function;)Lorg/apache/pekko/stream/javadsl/Flow;
    // parameter final  n
    // parameter final  f
   L0
    LINENUMBER 2260 L0
    ALOAD 0
    GETFIELD org/apache/pekko/stream/javadsl/Flow.delegate : Lorg/apache/pekko/stream/scaladsl/Flow;
    ALOAD 2
    INVOKEDYNAMIC apply(Lorg/apache/pekko/japi/function/Function;)Lscala/Function1; [
      // handle kind 0x6 : INVOKESTATIC
      java/lang/invoke/LambdaMetafactory.altMetafactory(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite;
      // arguments:
      (Ljava/lang/Object;)Ljava/lang/Object;, 
      // handle kind 0x6 : INVOKESTATIC
      org/apache/pekko/stream/javadsl/Flow.$anonfun$flatMapPrefix$1(Lorg/apache/pekko/japi/function/Function;Lscala/collection/immutable/Seq;)Lorg/apache/pekko/stream/scaladsl/Flow;, 
      (Lscala/collection/immutable/Seq;)Lorg/apache/pekko/stream/scaladsl/Flow;, 
      5, 
      1, 
      (Lscala/collection/immutable/Seq;)Lorg/apache/pekko/stream/scaladsl/Flow;
    ]
    ASTORE 5
    DUP
    IFNONNULL L1
    ACONST_NULL
    ATHROW
   L1
   FRAME FULL [org/apache/pekko/stream/javadsl/Flow I org/apache/pekko/japi/function/Function T T scala/Function1] [org/apache/pekko/stream/scaladsl/Flow]
    ASTORE 4
   L2
    NEW org/apache/pekko/stream/impl/fusing/FlatMapPrefix
    DUP
    ILOAD 1
    ALOAD 5
    INVOKESPECIAL org/apache/pekko/stream/impl/fusing/FlatMapPrefix.<init> (ILscala/Function1;)V
    ASTORE 6
   L3
    ALOAD 4
    ALOAD 6
    INVOKEVIRTUAL org/apache/pekko/stream/scaladsl/Flow.via (Lorg/apache/pekko/stream/Graph;)Lorg/apache/pekko/stream/scaladsl/Flow;
   L4
    ACONST_NULL
    ASTORE 6
   L5
    ACONST_NULL
    ASTORE 4
    ACONST_NULL
    ASTORE 5
    ASTORE 3
   L6
    LINENUMBER 2261 L6
    NEW org/apache/pekko/stream/javadsl/Flow
    DUP
    ALOAD 3
    INVOKESPECIAL org/apache/pekko/stream/javadsl/Flow.<init> (Lorg/apache/pekko/stream/scaladsl/Flow;)V
    ARETURN
   L7
    LOCALVARIABLE newDelegate Lorg/apache/pekko/stream/scaladsl/Flow; L6 L7 3
    LOCALVARIABLE this Lorg/apache/pekko/stream/javadsl/Flow; L0 L7 0
    LOCALVARIABLE n I L0 L7 1
    LOCALVARIABLE f Lorg/apache/pekko/japi/function/Function; L0 L7 2
    LOCALVARIABLE flatMapPrefix_this Lorg/apache/pekko/stream/scaladsl/Flow; L2 L5 4
    LOCALVARIABLE flatMapPrefix_f Lscala/Function1; L2 L5 5
    LOCALVARIABLE via_flow Lorg/apache/pekko/stream/Graph; L3 L4 6
    MAXSTACK = 4
    MAXLOCALS = 7

He-Pin · 2025-01-04T03:35:52Z

@pjfanning @raboof @mdedetrich Can we have this in 1.2.0 ?

pjfanning · 2025-01-04T09:06:56Z

I still think we should add new methods and keep the existing methods that accept Iterable as input. This is a breaking change and I like to avoid breaking changes.

He-Pin · 2025-01-04T09:17:17Z

What do you think about 2.0? As the list is just living in a lambda, I don't expect any breaking.

the function will always be erased.
it was iterable but now a list and a java list is always iterable.

So I don't expect any breaking with this change, as a daily Java developer, this is quite annoying, not many java developers save a function as a field and pass it to the function.

raboof · 2025-01-04T09:18:42Z

This change is binary compatible: any jar that was compiled against a Pekko version without this change will still work when ran against a Pekko version with this change.

It is also source compatible when passing in a lambda. It is not source compatible when passing in a function constant. That seems like an acceptable change.

users of Sets and classes like that might still want to use the method

The change of type is in the type of the objects Pekko provides, not in the type of the objects the user passes to Pekko. So changing the type from Iterable to List just formalizes a restriction on the Pekko implementation, not on the user code.

While it's arguably only a minor quality of life improvement, I'd be OK with merging this change. Since there's been a lot of discussion above I'd like to see more consensus on that before actually merging, though.

He-Pin · 2025-01-04T09:24:10Z

I'm using Java at $Work, the current Java api is really bad for me.

mdedetrich

I am also fine with merging this change, it's binary compatible.

However since it's new functionality it should be released when the minor version gets bumped, not patch version.

Waiting for 2.0.0 is not necessary/overkill

stream-tests/src/test/java/org/apache/pekko/stream/javadsl/SourceTest.java

...rs/1.0.x.backwards.excludes/pr-271-use-java-list-in-flatmapPrefix-javadsl.backwards.excludes

PR not ready yet, will re-review later

pjfanning

ok - if the consensus is to make the change, go ahead

cannot be backported to a patch release though, this needs a minor release

He-Pin added this to the 1.0.0 milestone Mar 24, 2023

He-Pin modified the milestones: 1.0.0, 1.1.0 Mar 24, 2023

mdedetrich added the question Further information is requested label Jun 17, 2023

apache deleted a comment from mdedetrich Jun 17, 2023

He-Pin added this to the 2.0.x milestone Jan 4, 2025

He-Pin added the t:stream Pekko Streams label Jan 4, 2025

He-Pin modified the milestones: 2.0.x, 1.2.0 Jan 4, 2025

He-Pin force-pushed the flatMapPrefix_javadsl branch 3 times, most recently from 25bfc58 to 5f0a2fa Compare January 4, 2025 04:10

He-Pin changed the title ~~!str Make flatMapPrefix javadsl using java.util.List~~ Make flatMapPrefix javadsl using java.util.List Jan 4, 2025

He-Pin requested review from mdedetrich, pjfanning, Roiocam and raboof January 4, 2025 04:11

He-Pin force-pushed the flatMapPrefix_javadsl branch from 5f0a2fa to 812770e Compare January 4, 2025 04:48

mdedetrich previously approved these changes Jan 4, 2025

View reviewed changes

pjfanning reviewed Jan 4, 2025

View reviewed changes

stream-tests/src/test/java/org/apache/pekko/stream/javadsl/SourceTest.java Outdated Show resolved Hide resolved

pjfanning reviewed Jan 4, 2025

View reviewed changes

...rs/1.0.x.backwards.excludes/pr-271-use-java-list-in-flatmapPrefix-javadsl.backwards.excludes Outdated Show resolved Hide resolved

mdedetrich self-requested a review January 4, 2025 09:38

chore: Make flatMapPrefix javadsl using java.util.List

3c2bb48

He-Pin force-pushed the flatMapPrefix_javadsl branch from 812770e to 3c2bb48 Compare January 4, 2025 09:47

He-Pin requested a review from pjfanning January 4, 2025 09:47

pjfanning approved these changes Jan 4, 2025

View reviewed changes

He-Pin merged commit c8ac6c0 into apache:main Jan 4, 2025
9 checks passed

He-Pin deleted the flatMapPrefix_javadsl branch January 4, 2025 15:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make flatMapPrefix javadsl using java.util.List #271

Make flatMapPrefix javadsl using java.util.List #271

He-Pin commented Mar 24, 2023 •

edited

Loading

pjfanning commented Mar 24, 2023

He-Pin commented Mar 24, 2023 •

edited

Loading

mdedetrich commented Jun 17, 2023

pjfanning commented Jun 17, 2023

mdedetrich commented Jun 17, 2023 •

edited

Loading

He-Pin commented Jun 17, 2023 •

edited

Loading

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

He-Pin commented Jun 17, 2023

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

mdedetrich commented Jun 17, 2023

mdedetrich commented Jun 18, 2023

pjfanning commented Jun 18, 2023 •

edited

Loading

mdedetrich commented Jun 18, 2023 •

edited

Loading

pjfanning commented Jun 18, 2023

mdedetrich commented Jun 18, 2023 •

edited

Loading

pjfanning commented Jun 18, 2023

mdedetrich commented Jun 18, 2023 •

edited

Loading

pjfanning commented Jun 18, 2023 •

edited

Loading

mdedetrich commented Jun 18, 2023

He-Pin commented Jan 5, 2024 •

edited

Loading

He-Pin commented Jan 4, 2025

pjfanning commented Jan 4, 2025

He-Pin commented Jan 4, 2025 •

edited

Loading

raboof commented Jan 4, 2025

He-Pin commented Jan 4, 2025

mdedetrich left a comment •

edited

Loading

pjfanning left a comment

Make flatMapPrefix javadsl using java.util.List #271

Make flatMapPrefix javadsl using java.util.List #271

Conversation

He-Pin commented Mar 24, 2023 • edited Loading

pjfanning commented Mar 24, 2023

He-Pin commented Mar 24, 2023 • edited Loading

mdedetrich commented Jun 17, 2023

pjfanning commented Jun 17, 2023

mdedetrich commented Jun 17, 2023 • edited Loading

He-Pin commented Jun 17, 2023 • edited Loading

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

He-Pin commented Jun 17, 2023

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

He-Pin commented Jun 17, 2023

pjfanning commented Jun 17, 2023

mdedetrich commented Jun 17, 2023

mdedetrich commented Jun 18, 2023

pjfanning commented Jun 18, 2023 • edited Loading

mdedetrich commented Jun 18, 2023 • edited Loading

pjfanning commented Jun 18, 2023

mdedetrich commented Jun 18, 2023 • edited Loading

pjfanning commented Jun 18, 2023

mdedetrich commented Jun 18, 2023 • edited Loading

pjfanning commented Jun 18, 2023 • edited Loading

mdedetrich commented Jun 18, 2023

He-Pin commented Jan 5, 2024 • edited Loading

He-Pin commented Jan 4, 2025

pjfanning commented Jan 4, 2025

He-Pin commented Jan 4, 2025 • edited Loading

raboof commented Jan 4, 2025

He-Pin commented Jan 4, 2025

mdedetrich left a comment • edited Loading

Choose a reason for hiding this comment

pjfanning left a comment

Choose a reason for hiding this comment

He-Pin commented Mar 24, 2023 •

edited

Loading

He-Pin commented Mar 24, 2023 •

edited

Loading

mdedetrich commented Jun 17, 2023 •

edited

Loading

He-Pin commented Jun 17, 2023 •

edited

Loading

pjfanning commented Jun 18, 2023 •

edited

Loading

mdedetrich commented Jun 18, 2023 •

edited

Loading

mdedetrich commented Jun 18, 2023 •

edited

Loading

mdedetrich commented Jun 18, 2023 •

edited

Loading

pjfanning commented Jun 18, 2023 •

edited

Loading

He-Pin commented Jan 5, 2024 •

edited

Loading

He-Pin commented Jan 4, 2025 •

edited

Loading

mdedetrich left a comment •

edited

Loading