ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Pavel Tupitsyn <ptupit...@apache.org>
Subject Re: IEP-70: Async Continuation Executor
Date Wed, 17 Mar 2021 14:01:11 GMT
> the user should use the right API

Not a good excuse really. We have a usability problem, you have to admit it.
"The brakes did not work on your car - too bad, you should have known that
on Sundays only your left foot is allowed on the pedal"

This particular use case is too intricate.
Even when you know about that, it is difficult to decide what can run on
the striped pool,
and what can't. It is too easy to forget.
And most people don't know, even among Ignite developers.

Documentation won't help - no one is going to read the Javadoc for a
trivial method like putAsync.


So I propose to have a safe default.
Then document the performance tuning opportunity on [1].

Think about how many users abandon a product because it mysteriously
crashes and hangs.

[1]
https://ignite.apache.org/docs/latest/perf-and-troubleshooting/general-perf-tips


On Wed, Mar 17, 2021 at 4:21 PM Вячеслав Коптилин <slava.koptilin@gmail.com>
wrote:

> Hi Pavel,
>
> Well, I think that the user should use the right API instead of introducing
> uncontested overhead for everyone.
> For instance, the code that is provided by IEP can changed as follows:
>
> IgniteFuture fut = cache.putAsync(1, 1);
> fut.listenAync(f -> {
>     // Executes on Striped pool and deadlocks.
>     cache.replace(1, 2);
> }, ForkJoinPool.commonPool());
>
> Of course, it does not mean that this fact should not be properly
> documented.
> Perhaps, I am missing something.
>
> Thanks,
> S.
>
> ср, 17 мар. 2021 г. в 16:01, Pavel Tupitsyn <ptupitsyn@apache.org>:
>
> > Slava,
> >
> > Your suggestion is to keep things as is and discard the IEP, right?
> >
> > >  this can lead to significant overhead
> > Yes, there is some overhead, but the cost of accidentally starving the
> > striped pool is worse,
> > not to mention the deadlocks.
> >
> > I believe that we should favor correctness over performance in any case.
> >
> >
> > On Wed, Mar 17, 2021 at 3:34 PM Вячеслав Коптилин <
> > slava.koptilin@gmail.com>
> > wrote:
> >
> > > Well, the specified method already exists :)
> > >
> > >     /**
> > >      * Registers listener closure to be asynchronously notified
> whenever
> > > future completes.
> > >      * Closure will be processed in specified executor.
> > >      *
> > >      * @param lsnr Listener closure to register. Cannot be {@code
> null}.
> > >      * @param exec Executor to run listener. Cannot be {@code null}.
> > >      */
> > >     public void listenAsync(IgniteInClosure<? super IgniteFuture<V>>
> > lsnr,
> > > Executor exec);
> > >
> > > Thanks,
> > > S.
> > >
> > > ср, 17 мар. 2021 г. в 15:25, Вячеслав Коптилин <
> slava.koptilin@gmail.com
> > >:
> > >
> > > > Hello Pavel,
> > > >
> > > > I took a look at your IEP and pool request. I have the following
> > > concerns.
> > > > First of all, this change breaks the contract of
> > > IgniteFuture#listen(lsnr)
> > > >
> > > >     /**
> > > >      * Registers listener closure to be asynchronously notified
> > whenever
> > > > future completes.
> > > >      * Closure will be processed in thread that completes this future
> > or
> > > > (if future already
> > > >      * completed) immediately in current thread.
> > > >      *
> > > >      * @param lsnr Listener closure to register. Cannot be {@code
> > null}.
> > > >      */
> > > >     public void listen(IgniteInClosure<? super IgniteFuture<V>>
> lsnr);
> > > >
> > > >     In your pull request, the listener is always called from a
> > specified
> > > > thread pool (which is fork-join by default)
> > > >     even though the future is already completed at the moment the
> > listen
> > > > method is called.
> > > >     In my opinion, this can lead to significant overhead - submission
> > > > requires acquiring a lock and notifying a pool thread.
> > > >
> > > >     It seems to me, that we should not change the current behavior.
> > > > However, thread pool executor can be added as an optional parameter
> of
> > > > listen() method as follows:
> > > >
> > > >         public void listen(IgniteInClosure<? super IgniteFuture<V>>
> > lsnr,
> > > > Executor exec);
> > > >
> > > > Thanks,
> > > > S.
> > > >
> > > > пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <ptupitsyn@apache.org>:
> > > >
> > > >> Igniters,
> > > >>
> > > >> Please review the IEP [1] and let me know your thoughts.
> > > >>
> > > >> [1]
> > > >>
> > > >>
> > >
> >
> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
> > > >>
> > > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message