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
>
