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 <ptupit...@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.kopti...@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.kopti...@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 <ptupit...@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
> > >>
> > >
> >
>
