pekko icon indicating copy to clipboard operation
pekko copied to clipboard

!str Make flatMapPrefix javadsl using java.util.List

Open He-Pin opened this issue 2 years ago • 34 comments

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

He-Pin avatar Mar 24 '23 17:03 He-Pin

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

pjfanning avatar Mar 24 '23 18:03 pjfanning

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

He-Pin avatar Mar 24 '23 18:03 He-Pin

@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?

mdedetrich avatar Jun 17 '23 12:06 mdedetrich

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.

pjfanning avatar Jun 17 '23 12:06 pjfanning

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.

mdedetrich avatar Jun 17 '23 12:06 mdedetrich

@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 avatar Jun 17 '23 15:06 He-Pin

Screenshot_20230617_235348_GitHub.jpg

And this is another reason.

He-Pin avatar Jun 17 '23 15:06 He-Pin

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.

pjfanning avatar Jun 17 '23 15:06 pjfanning

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 avatar Jun 17 '23 16:06 He-Pin

@mdedetrich It instead liberating the usage on Java side. Screenshot_20230618_000434_GitHub.jpg

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

He-Pin avatar Jun 17 '23 16:06 He-Pin

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.

pjfanning avatar Jun 17 '23 16:06 pjfanning

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.

He-Pin avatar Jun 17 '23 16:06 He-Pin

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.

pjfanning avatar Jun 17 '23 16:06 pjfanning

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

He-Pin avatar Jun 17 '23 16:06 He-Pin

image

pjfanning avatar Jun 17 '23 16:06 pjfanning

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 avatar Jun 17 '23 16:06 mdedetrich

@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).

mdedetrich avatar Jun 18 '23 11:06 mdedetrich

@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).

pjfanning avatar Jun 18 '23 11:06 pjfanning

@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

mdedetrich avatar Jun 18 '23 12:06 mdedetrich

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.

pjfanning avatar Jun 18 '23 12:06 pjfanning

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.

mdedetrich avatar Jun 18 '23 12:06 mdedetrich

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.

pjfanning avatar Jun 18 '23 13:06 pjfanning

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.

mdedetrich avatar Jun 18 '23 13:06 mdedetrich

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.

pjfanning avatar Jun 18 '23 13:06 pjfanning

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.

mdedetrich avatar Jun 18 '23 13:06 mdedetrich

@pjfanning @mdedetrich Can we do this in the 1.1.0 version?

He-Pin avatar Dec 24 '23 06:12 He-Pin

Even though the function parameter is an iterable, we passed a Seq while invoking it,and I checked the relevant tests and found that they were all list . Perhaps users are more accustomed to using lists,or this could be a design errors in the API.

laglangyue avatar Dec 25 '23 05:12 laglangyue

I remain -1 on changing the request signature of the method. We can add new methods and deprecate the old ones.

pjfanning avatar Dec 25 '23 08:12 pjfanning

What about the new method name? switchOnFirst , prefixFlatmap?

He-Pin avatar Dec 25 '23 08:12 He-Pin

What about the new method name? prefixAndFlatmap?

Thinking about it again, it might be worth keeping the flatMapPrefix without deprecating it - users of Sets and classes like that might still want to use the method. How about calling the new method flatMapPrefixList to make it more obvious that it is related to flatMapPrefix?

pjfanning avatar Dec 25 '23 08:12 pjfanning