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 > > > > >> > > > > > > > > > > > > > > >
