Pavel,
I tried this:
@Test
public void test() throws Exception {
IgniteCache<Integer, String> cache =
startGrid().getOrCreateCache("test_cache");
cache.putAsync(1, "one").listen(f -> cache.replace(1, "two"));
assertEquals("two", cache.get(1));
}
and this test is green.
I believe that an user can make listener that leads to deadlock, but
the example in the IEP does not reflect this.
ср, 17 мар. 2021 г. в 17:36, Вячеслав Коптилин <slava.kopti...@gmail.com>:
> Hi Pavel,
>
> > Not a good excuse really. We have a usability problem, you have to admit
> it.
> Fair enough. I agree that this is a usability issue, but I have doubts that
> the proposed approach to overcome it is the best one.
>
> > Documentation won't help - no one is going to read the Javadoc for a
> trivial method like putAsync
> That is sad... However, I don't think that this is a strong argument here.
>
> This is just my opinion. Let's see what other community members have to
> say.
>
> Thanks,
> S.
>
>
> ср, 17 мар. 2021 г. в 17:01, Pavel Tupitsyn <ptupit...@apache.org>:
>
> > > 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.kopti...@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 <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
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>
