mdedetrich commented on PR #2684: URL: https://github.com/apache/pekko/pull/2684#issuecomment-3976627012
> > One possibility is to introduce a boolean parameter on the futureSink method to say if want to support empty streams. In 1.x, we can default to false and in 2.x we can change this to true. > > Would a defaulted new param cause bin compat issues in 1.x? If it does then this probably a non-starter. > > You'd need to overload the method with the extra param. Just adding a new param with a default value would change the method signature. If you have to add the new method anyway I feel like an explicitly-named method is better. > > Also I feel like the lazy/eager semantics are important to communicate in the naming, since that's what the user cares about when choosing the operator. Otherwise they would just always choose the version that can handle empty streams. > > > In 2.x, we can choose to make a major change but if we want to get this into 1.5.0, we need a low impact change. Maybe we could focus on a 1.x friendly change first and then discuss if we want to make a bigger change in 2.x. > > Absolutely, that's why I proposed a new name. In 2.x maybe we could just remove `futureSink` and keep `eagerFutureSink`/`lazyFutureSink`. The naming would be inconsistent with the rest of the API but at least it would be clear. So it is to be said that at least in pekko-streams there are 2 overloaded definitions of "lazy". One is an anonymous function that returns a future, with the anonymous function being deliberate as the future should only be executed at the specified time, hency the lazy and the other definition is, as you said, deferring the materialization. There are 2 options we have here, one is that this change only goes in to Pekko 2.x where we reuse the `futureSink` method. The advantage of this change is that we will have completely consistent naming (which I think is a huge benefit) however the downsides is that there will be a behaviour change (even though this is allowed in 2.x) and the improvement will only be in Pekko 2.x. If we do this we would have to add a deprecation warning in Pekko 1.x telling users not to use `futureSink` as the behaviour will change in the future. The other option is that as you said, we use a new `eagerFutureSink` and deprecate + remove `futureSink` in a later time. I have a slight preference for the former although admittedly it would require a bit more work. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
