Guys,
I want to remember there is one more point to pay attention to.
Extending Future and CompletableStage is more than just prevents unexpected
behavior if a user completed the future.
First of all, it helps us to write safer code as we won't a method contract
exposed such methods as to a user as to a developer.
If you look at Ignite-2 code, you may found a number of places where we
return e.g. exchange futures or partition release futures.
Assume the impact if we will return CompletableFuture instead, which can be
completed in 3-rd party plugin by mistake?
The suggested approach allows us to don't bother if a CompletableFuture has
to be wrapped or not.
On Tue, Mar 30, 2021 at 12:22 PM Alexey Goncharuk <
alexey.goncha...@gmail.com> wrote:
> Ivan,
>
> My concern with the concept of a user completing the future returned from
> Ignite public API is that it is unclear how to interpret this action (this
> backs Val's message).
> Let's say a user started a compute with fut = compute.runAsync(task); and
> now calls fut.complete(someVal); Does this mean that Ignite no longer needs
> to execute the task? If the task is currently running, does it need to be
> canceled?
>
> Using CompletableFuture.anyOf() is a good instrument in this case because
> it makes the 'first future wins' contract explicit in the code. Besides
> that, the point regarding the cancel() method is valid, and we will need
> some custom mechanics to cancel a computation, so a custom interface that
> simply extends both Future and CompletableStage seems reasonable to me.
>
> --AG
>
> пн, 29 мар. 2021 г. в 09:12, Ivan Pavlukhin <vololo...@gmail.com>:
>
> > Val,
> >
> > There were enough hype around Reactive programming past years. I
> > remind a lot of talks about RxJava. And I suppose it worth to consider
> > it. But it requires some time to study modern trends to make a choice.
> > So far I am not ready to facilitate Reactive API for Ignite 3.
> >
> > Regarding CompletableFuture.
> >
> > > The point is that currently a future returned from any of Ignite's
> async
> > > operations is supposed to be completed with a value only by Ignite
> > itself,
> > > not by the user. If we follow the same approach in Ignite 3, returning
> > > CompletableFuture is surely wrong in my view.
> >
> > My first thoughts was similar. But later I thought what a user would
> > like do with returned future. And one of cases I imagined was a case
> > of alternative result. E.g. a user uses Ignite and another data source
> > in his application. He wants to use a value arrived faster. He
> > combines 2 futures like CompletableFuture.anyOf(...). Consequently
> > even if we prohibit CompletableFuture.complete(...) explicitly then it
> > will be possible to create a combination that will allow premature
> > future completion. After all generally CompletableFuture is a
> > placeholder for async computaion result and if a user wants to
> > substitute result returned from Ignite why should we disallow him to
> > do it?
> >
> > Also I found one more suspicious thing with CompletableFuture. As it
> > is a concrete class it implements a cancel() method. And as I see the
> > implementation does not try to cancel underlying computations. Is not
> > it a problem?
> >
> > 2021-03-29 7:30 GMT+03:00, Valentin Kulichenko <
> > valentin.kuliche...@gmail.com>:
> > > Ivan,
> > >
> > > It's not really about the "harm", but more about "what should we do if
> > this
> > > method is called?". Imagine the following code:
> > >
> > > CompletableFuture<String> fut = cache.getAsync(key);
> > > fut.complete("something");
> > >
> > > What should happen in this case?
> > >
> > > The point is that currently a future returned from any of Ignite's
> async
> > > operations is supposed to be completed with a value only by Ignite
> > itself,
> > > not by the user. If we follow the same approach in Ignite 3, returning
> > > CompletableFuture is surely wrong in my view.
> > >
> > > At the same time, if we take a fundamentally different route with the
> > async
> > > APIs, this whole discussion might become irrelevant. For example, can
> you
> > > elaborate on your thinking around the reactive API? Do you have any
> > > specifics in mind?
> > >
> > > -Val
> > >
> > > On Sat, Mar 27, 2021 at 9:18 PM Ivan Pavlukhin <vololo...@gmail.com>
> > wrote:
> > >
> > >> > The methods below shouldn't be accessible for user:
> > >> > complete()
> > >> > completeExceptionaly()
> > >>
> > >> Folks, in case of user-facing API, do you think there is a real harm
> > >> in allowing a user to manually "complete" a future? I suppose a user
> > >> employs some post-processing for future results and potentially wants
> > >> to have control of these results as well. E.g. premature completion in
> > >> case when a result is no longer needed is possible usage.
> > >>
> > >> Also I thinkg it might be a good time to ponder about Future/Promise
> > >> APIs in general. Why such API is our choice? Can we choose e.g.
> > >> Reactive API style instead?
> > >>
> > >> 2021-03-27 0:33 GMT+03:00, Valentin Kulichenko <
> > >> valentin.kuliche...@gmail.com>:
> > >> > Andrey,
> > >> >
> > >> > I see. So in a nutshell, you're saying that we want to return a
> future
> > >> that
> > >> > the user's code is not allowed to complete. In this case, I think
> it's
> > >> > clear that CompletableFuture is not what we need. We actually need a
> > >> > NonCompletableFuture :)
> > >> >
> > >> > My vote is for the custom interface.
> > >> >
> > >> > -Val
> > >> >
> > >> > On Fri, Mar 26, 2021 at 2:25 AM Andrey Mashenkov
> > >> > <andrey.mashen...@gmail.com>
> > >> > wrote:
> > >> >
> > >> >> Val,
> > >> >>
> > >> >> The methods below shouldn't be accessible for user:
> > >> >> complete()
> > >> >> completeExceptionaly()
> > >> >>
> > >> >> Returning CompletableFuture we must always make a copy to prevent
> the
> > >> >> original future from being completed by mistake.
> > >> >> I think it will NOT be enough to do that returing the future to the
> > >> >> end-user, but from every critical module to the outside of the
> > module,
> > >> >> e.g. to plugins. The impact of disclosing ExchangeFuture,
> > >> >> PartitionReleaseFuture to plugins may be serious.
> > >> >>
> > >> >> IgniteFuture<T> extends Future<T>, CompletionStage<T> which
> > >> >> implementation
> > >> >> will just wrap CompletableFuture these issues will be resolved in
> > >> natural
> > >> >> way.
> > >> >> In addition we can force toCompletableFuture() method to return a
> > >> >> defensive
> > >> >> copy(), that resolves the last concern.
> > >> >>
> > >> >>
> > >> >> On Fri, Mar 26, 2021 at 11:38 AM Konstantin Orlov
> > >> >> <kor...@gridgain.com>
> > >> >> wrote:
> > >> >>
> > >> >> > CompletableFuture seems a better option to me.
> > >> >> >
> > >> >> > --
> > >> >> > Regards,
> > >> >> > Konstantin Orlov
> > >> >> >
> > >> >> >
> > >> >> >
> > >> >> >
> > >> >> > > On 26 Mar 2021, at 11:07, Pavel Tupitsyn <ptupit...@apache.org
> >
> > >> >> > > wrote:
> > >> >> > >
> > >> >> > > On the one hand, I agree with Alexey.
> > >> >> > > CompletableFuture has complete* methods which should not be
> > >> available
> > >> >> to
> > >> >> > > the user code.
> > >> >> > > This can be solved with a simple interface like we do in Thin
> > >> Client:
> > >> >> > > IgniteClientFuture<T> extends Future<T>, CompletionStage<T>
> > >> >> > >
> > >> >> > >
> > >> >> > > On the other hand:
> > >> >> > > - CompletionStage has toCompletableFuture anyway (rather weird)
> > >> >> > > - Other libraries use CompletableFuture and it seems to be fine
> > >> >> > > - Using CompletableFuture is the simplest approach
> > >> >> > >
> > >> >> > >
> > >> >> > > So I lean towards CompletableFuture too.
> > >> >> > >
> > >> >> > > On Fri, Mar 26, 2021 at 10:46 AM Alexey Kukushkin <
> > >> >> > kukushkinale...@gmail.com>
> > >> >> > > wrote:
> > >> >> > >
> > >> >> > >> I do not like Java's CompletableFuture and prefer our own
> Future
> > >> >> > (revised
> > >> >> > >> IgniteFuture).
> > >> >> > >>
> > >> >> > >> My understanding of the Future (or Promise) pattern in general
> > is
> > >> >> having
> > >> >> > >> two separate APIs:
> > >> >> > >>
> > >> >> > >> 1. Server-side: create, set result, raise error, cancel from
> > >> >> > >> server.
> > >> >> > >> 2. Client-side: get result, handle error, cancel from client
> > >> >> > >>
> > >> >> > >> Java's CompletableFuture looks like both the client-side and
> > >> >> > >> server-side API. The "Completeable" prefix in the name is
> > already
> > >> >> > confusing
> > >> >> > >> for a client since it cannot "complete" an operation, only a
> > >> >> > >> server
> > >> >> can.
> > >> >> > >>
> > >> >> > >> I would create our own IgniteFuture adding client-side
> > >> functionality
> > >> >> we
> > >> >> > >> currently miss (like client-side cancellation).
> > >> >> > >>
> > >> >> > >>
> > >> >> > >> пт, 26 мар. 2021 г. в 01:08, Valentin Kulichenko <
> > >> >> > >> valentin.kuliche...@gmail.com>:
> > >> >> > >>
> > >> >> > >>> Andrey,
> > >> >> > >>>
> > >> >> > >>> Can you compile a full list of these risky methods, and
> > >> >> > >>> elaborate
> > >> >> > >>> on
> > >> >> > what
> > >> >> > >>> the risks are?
> > >> >> > >>>
> > >> >> > >>> Generally, CompletableFuture is a much better option, because
> > >> >> > >>> it's
> > >> >> > >>> standard. But we need to make sure it actually fits our needs
> > >> >> > >>> and
> > >> >> > doesn't
> > >> >> > >>> do more harm than good.
> > >> >> > >>>
> > >> >> > >>> -Val
> > >> >> > >>>
> > >> >> > >>> On Thu, Mar 25, 2021 at 12:23 PM Alexei Scherbakov <
> > >> >> > >>> alexey.scherbak...@gmail.com> wrote:
> > >> >> > >>>
> > >> >> > >>>> I think both options are fine, but personally lean toward
> > >> >> > >>>> CompletableFuture.
> > >> >> > >>>>
> > >> >> > >>>> чт, 25 мар. 2021 г. в 17:56, Atri Sharma <a...@apache.org>:
> > >> >> > >>>>
> > >> >> > >>>>> I would suggest using CompletableFuture -- I don't see a
> need
> > >> for
> > >> >> > >>>>> a
> > >> >> > >>>> custom
> > >> >> > >>>>> interface that is unique to us.
> > >> >> > >>>>>
> > >> >> > >>>>> It also allows a lower barrier for new contributors for
> > >> >> understanding
> > >> >> > >>>>> existing code
> > >> >> > >>>>>
> > >> >> > >>>>> On Thu, 25 Mar 2021, 20:18 Andrey Mashenkov, <
> > >> >> > >>> andrey.mashen...@gmail.com
> > >> >> > >>>>>
> > >> >> > >>>>> wrote:
> > >> >> > >>>>>
> > >> >> > >>>>>> Hi Igniters,
> > >> >> > >>>>>>
> > >> >> > >>>>>> I'd like to start a discussion about replacing our custom
> > >> >> > >>> IgniteFuture
> > >> >> > >>>>>> class with CompletableFuture - existed JDK class
> > >> >> > >>>>>> or rework it's implementation (like some other products
> > done)
> > >> to
> > >> >> > >>>>>> a
> > >> >> > >>>>>> composition of CompletionStage and Future interfaces.
> > >> >> > >>>>>> or maybe other option if you have any ideas. Do you?
> > >> >> > >>>>>>
> > >> >> > >>>>>> 1. The first approach pros and cons are
> > >> >> > >>>>>> + Well-known JDK class
> > >> >> > >>>>>> + Already implemented
> > >> >> > >>>>>> - It is a class, not an interface.
> > >> >> > >>>>>> - Expose some potentially harmful methods like
> "complete()".
> > >> >> > >>>>>>
> > >> >> > >>>>>> On the other side, it has copy() method to create
> defensive
> > >> copy
> > >> >> > >> and
> > >> >> > >>>>>> minimalCompletionStage() to restrict harmful method usage.
> > >> >> > >>>>>> Thus, this look like an applicable solution, but we should
> > be
> > >> >> > >> careful
> > >> >> > >>>>>> exposing internal future to the outside.
> > >> >> > >>>>>>
> > >> >> > >>>>>> 2. The second approach is to implement our own interface
> > like
> > >> >> > >>>>>> the
> > >> >> > >>> next
> > >> >> > >>>>> one:
> > >> >> > >>>>>>
> > >> >> > >>>>>> interface IgniteFuture<T> extends CompletableStage<T>,
> > >> Future<T>
> > >> >> > >>>>>> {
> > >> >> > >>>>>> }
> > >> >> > >>>>>>
> > >> >> > >>>>>> Pros and cons are
> > >> >> > >>>>>> + Our interfaces/classes contracts will expose an
> interface
> > >> >> > >>>>>> rather
> > >> >> > >>> than
> > >> >> > >>>>>> concrete implementation.
> > >> >> > >>>>>> + All methods are safe.
> > >> >> > >>>>>> - Some implementation is required.
> > >> >> > >>>>>> - CompletableStage has a method toCompletableFuture() and
> > can
> > >> be
> > >> >> > >>>>> converted
> > >> >> > >>>>>> to CompletableFuture. This should be supported.
> > >> >> > >>>>>>
> > >> >> > >>>>>> However, we still could wrap CompletableFuture and don't
> > >> >> > >>>>>> bother
> > >> >> > >> about
> > >> >> > >>>>>> creating a defensive copy.
> > >> >> > >>>>>>
> > >> >> > >>>>>>
> > >> >> > >>>>>> Other project experience:
> > >> >> > >>>>>> * Spotify uses CompletableFuture directly [1].
> > >> >> > >>>>>> * Redis goes the second approach [2]
> > >> >> > >>>>>> * Vertx explicitly extends CompletableFuture [3]. However,
> > >> >> > >>>>>> they
> > >> >> > >> have
> > >> >> > >>>>> custom
> > >> >> > >>>>>> future classes and a number of helpers that could be
> > replaced
> > >> >> > >>>>>> with
> > >> >> > >>>>>> CompletableStage. Maybe it is just a legacy.'
> > >> >> > >>>>>>
> > >> >> > >>>>>> Any thoughts?
> > >> >> > >>>>>>
> > >> >> > >>>>>> [1]
> > >> >> > >>>>>>
> > >> >> > >>>>>>
> > >> >> > >>>>>
> > >> >> > >>>>
> > >> >> > >>>
> > >> >> > >>
> > >> >> >
> > >> >>
> > >>
> >
> https://spotify.github.io/completable-futures/apidocs/com/spotify/futures/ConcurrencyReducer.html
> > >> >> > >>>>>> [2]
> > >> >> > >>>>>>
> > >> >> > >>>>>>
> > >> >> > >>>>>
> > >> >> > >>>>
> > >> >> > >>>
> > >> >> > >>
> > >> >> >
> > >> >>
> > >>
> >
> https://lettuce.io/lettuce-4/release/api/com/lambdaworks/redis/RedisFuture.html
> > >> >> > >>>>>> [3]
> > >> >> > >>>>>>
> > >> >> > >>>>>>
> > >> >> > >>>>>
> > >> >> > >>>>
> > >> >> > >>>
> > >> >> > >>
> > >> >> >
> > >> >>
> > >>
> >
> https://javadoc.io/static/org.jspare.vertx/vertx-jspare/1.1.0-M03/org/jspare/vertx/concurrent/VertxCompletableFuture.html
> > >> >> > >>>>>> --
> > >> >> > >>>>>> Best regards,
> > >> >> > >>>>>> Andrey V. Mashenkov
> > >> >> > >>>>>>
> > >> >> > >>>>>
> > >> >> > >>>>
> > >> >> > >>>>
> > >> >> > >>>> --
> > >> >> > >>>>
> > >> >> > >>>> Best regards,
> > >> >> > >>>> Alexei Scherbakov
> > >> >> > >>>>
> > >> >> > >>>
> > >> >> > >>
> > >> >> > >>
> > >> >> > >> --
> > >> >> > >> Best regards,
> > >> >> > >> Alexey
> > >> >> > >>
> > >> >> >
> > >> >> >
> > >> >>
> > >> >> --
> > >> >> Best regards,
> > >> >> Andrey V. Mashenkov
> > >> >>
> > >> >
> > >>
> > >>
> > >> --
> > >>
> > >> Best regards,
> > >> Ivan Pavlukhin
> > >>
> > >
> >
> >
> > --
> >
> > Best regards,
> > Ivan Pavlukhin
> >
>
--
Best regards,
Andrey V. Mashenkov
