Stan, I'm ok with using public pool by default, but we need a way to restore the old behavior, do you agree? I think we should keep the new IgniteConfiguration property.
On Fri, Mar 26, 2021 at 2:12 PM Alexei Scherbakov < alexey.scherbak...@gmail.com> wrote: > Pavel, > > Dedicated pool looks safer and more manageable to me. Make sure the threads > in the pool are lazily started and stopped if not used for some time. > > Because I have no more real arguments against the change, I suggest to > proceed with this approach. > > чт, 25 мар. 2021 г. в 22:16, Pavel Tupitsyn <ptupit...@apache.org>: > > > Alexei, > > > > > we already have ways to control a listener's behavior > > No, we don't have a way to fix current broken and dangerous behavior > > globally. > > You should not expect the user to fix every async call manually. > > > > > commonPool can alter existing deployments in unpredictable ways, > > > if commonPool is heavily used for other purposes > > Common pool resizes dynamically to accommodate the load [1] > > What do you think about Stan's suggestion to use our public pool instead? > > > > [1] > > > > > https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ForkJoinPool.html > > > > On Thu, Mar 25, 2021 at 10:10 PM Pavel Tupitsyn <ptupit...@apache.org> > > wrote: > > > > > > I don't agree that the code isn't related to Ignite - it is something > > > that the user does via Ignite API > > > > > > This is a misconception. When you write general-purpose async code, it > > > looks like this: > > > > > > myClass.fooAsync() > > > .chain(igniteCache.putAsync) > > > .chain(myClass.barAsync) > > > .chain(...) > > > > > > And so on, you jump from one continuation to another. > > > You don't think about this as "I use Ignite API to run my > continuation", > > > this is just another async call among hundreds of others. > > > > > > And you don't want 1 of 20 libraries that you use to have "special > needs" > > > like Ignite does right now. > > > > > > I know Java is late to the async party and not everyone is used to this > > > mindset, > > > but the situation changes, more and more code bases go async all the > way, > > > use CompletionStage everywhere, etc. > > > > > > > > > > If we go with the public pool - no additional options needed. > > > > > > I guess public pool should work. > > > However, I would prefer to keep using commonPool, which is recommended > > for > > > a general purpose like this. > > > > > > On Thu, Mar 25, 2021 at 3:56 PM Alexei Scherbakov < > > > alexey.scherbak...@gmail.com> wrote: > > > > > >> Pavel, > > >> > > >> The change still looks a bit risky to me, because the default executor > > is > > >> set to commonPool and can alter existing deployments in unpredictable > > >> ways, > > >> if commonPool is heavily used for other purposes. > > >> > > >> Runnable::run usage is not obvious as well and should be properly > > >> documented as a way to return to old behavior. > > >> > > >> I'm not sure we need it in 2.X for the reasons above - we already have > > >> ways > > >> to control a listener's behavior - it's a matter of good documentation > > to > > >> me. > > >> > > >> > > >> > > >> > > >> > > >> > > >> чт, 25 мар. 2021 г. в 15:33, Pavel Tupitsyn <ptupit...@apache.org>: > > >> > > >> > Alexei, > > >> > > > >> > > Sometimes it's more desirable to execute the listener in the same > > >> thread > > >> > > It's up to the user to decide. > > >> > > > >> > Yes, we give users a choice to configure the executor as > Runnable::run > > >> and > > >> > use the same thread if needed. > > >> > However, it should not be the default behavior as explained above > (bad > > >> > usability, unexpected major issues). > > >> > > > >> > On Thu, Mar 25, 2021 at 3:06 PM Alexei Scherbakov < > > >> > alexey.scherbak...@gmail.com> wrote: > > >> > > > >> > > Pavel, > > >> > > > > >> > > While I understand the issue and overall agree with you, I'm > against > > >> the > > >> > > execution of listeners in separate thread pool by default. > > >> > > > > >> > > Sometimes it's more desirable to execute the listener in the same > > >> thread, > > >> > > for example if it's some lightweight closure. > > >> > > > > >> > > It's up to the user to decide. > > >> > > > > >> > > I think the IgniteFuture.listen method should be properly > documented > > >> to > > >> > > avoid execution of cluster operations or any other potentially > > >> blocking > > >> > > operations inside the listener. > > >> > > > > >> > > Otherwise listenAsync should be used. > > >> > > > > >> > > > > >> > > > > >> > > чт, 25 мар. 2021 г. в 14:04, Pavel Tupitsyn <ptupit...@apache.org > >: > > >> > > > > >> > > > Stan, > > >> > > > > > >> > > > We have thread pools dedicated for specific purposes, like cache > > >> > > (striped), > > >> > > > compute (pub), query, etc > > >> > > > As I understand it, the reason here is to limit the number of > > >> threads > > >> > > > dedicated to a given subsystem. > > >> > > > For example, Compute may be overloaded with work, but Cache and > > >> > Discovery > > >> > > > will keep going. > > >> > > > > > >> > > > This is different from async continuations, which are arbitrary > > user > > >> > > code. > > >> > > > So what is the benefit of having a new user pool for arbitrary > > code > > >> > that > > >> > > is > > >> > > > probably not related to Ignite at all? > > >> > > > > > >> > > > On Thu, Mar 25, 2021 at 1:31 PM <stanlukya...@gmail.com> wrote: > > >> > > > > > >> > > > > Pavel, > > >> > > > > > > >> > > > > This is a great work, fully agree with the overall idea and > > >> approach. > > >> > > > > > > >> > > > > However, I have some reservations about the API. We sure do > > have a > > >> > lot > > >> > > of > > >> > > > > async stuff in the system, and I would suggest to stick to the > > >> usual > > >> > > > design > > >> > > > > - create a separate thread pool, add a single property to > > control > > >> the > > >> > > > size > > >> > > > > of the pool. > > >> > > > > Alternatively, we may consider using public pool for that. > May I > > >> ask > > >> > if > > >> > > > > there is an example use case which doesn’t work with public > > pool? > > >> > > > > > > >> > > > > For .NET, agree that we should follow the rules and APIs of > the > > >> > > platform, > > >> > > > > so the behavior might slightly differ. > > >> > > > > > > >> > > > > Thanks, > > >> > > > > Stan > > >> > > > > > > >> > > > > > On 24 Mar 2021, at 09:52, Pavel Tupitsyn < > > ptupit...@apache.org> > > >> > > wrote: > > >> > > > > > > > >> > > > > > Igniters, since there are no more comments and/or review > > >> feedback, > > >> > > > > > I'm going to merge the changes by the end of the week. > > >> > > > > > > > >> > > > > >> On Mon, Mar 22, 2021 at 10:37 PM Pavel Tupitsyn < > > >> > > ptupit...@apache.org > > >> > > > > > > >> > > > > >> wrote: > > >> > > > > >> > > >> > > > > >> Ready for review: > > >> > > > > >> https://github.com/apache/ignite/pull/8870 > > >> > > > > >> > > >> > > > > >> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn < > > >> > > ptupit...@apache.org> > > >> > > > > >> wrote: > > >> > > > > >> > > >> > > > > >>> Simple benchmark added - see JmhCacheAsyncListenBenchmark > in > > >> the > > >> > > PR. > > >> > > > > >>> There is a 6-8% drop (1 client, 2 servers, 1 machine, int > > >> > key/val). > > >> > > > > >>> I expect this difference to become barely observable on > > >> > real-world > > >> > > > > >>> workloads. > > >> > > > > >>> > > >> > > > > >>> On Thu, Mar 18, 2021 at 12:35 PM Pavel Tupitsyn < > > >> > > > ptupit...@apache.org> > > >> > > > > >>> wrote: > > >> > > > > >>> > > >> > > > > >>>> Denis, > > >> > > > > >>>> > > >> > > > > >>>> For a reproducer, please see > > >> > > CacheAsyncContinuationExecutorTest.java > > >> > > > > in > > >> > > > > >>>> the linked PoC [1] > > >> > > > > >>>> > > >> > > > > >>>> [1] > > >> > > > > >>>> > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > https://github.com/apache/ignite/pull/8870/files#diff-c788c12013622093df07d8f628a6e8c5fb0c15007f0787f2d459dbb3e377fc5aR54 > > >> > > > > >>>> > > >> > > > > >>>> On Thu, Mar 18, 2021 at 1:58 AM Raymond Wilson < > > >> > > > > >>>> raymond_wil...@trimble.com> wrote: > > >> > > > > >>>> > > >> > > > > >>>>> We implemented the ContinueWith() suggestion from Pavel, > > >> and it > > >> > > > works > > >> > > > > >>>>> well > > >> > > > > >>>>> so far in testing, though we do not have data to support > > if > > >> > there > > >> > > > is > > >> > > > > or > > >> > > > > >>>>> is > > >> > > > > >>>>> not a performance penalty in our use case.. > > >> > > > > >>>>> > > >> > > > > >>>>> To lend another vote to the 'Don't do continuations on > the > > >> > > striped > > >> > > > > >>>>> thread > > >> > > > > >>>>> pool' line of thinking: Deadlocking is an issue as is > > >> > starvation. > > >> > > > In > > >> > > > > >>>>> some > > >> > > > > >>>>> ways starvation is more insidious because by the time > > things > > >> > stop > > >> > > > > >>>>> working > > >> > > > > >>>>> the cause and effect distance may be large. > > >> > > > > >>>>> > > >> > > > > >>>>> I appreciate the documentation does make statements > about > > >> not > > >> > > > > performing > > >> > > > > >>>>> cache operations in a continuation due to deadlock > > >> > possibilities, > > >> > > > but > > >> > > > > >>>>> that > > >> > > > > >>>>> statement does not reveal why this is an issue. It is > > less a > > >> > case > > >> > > > of > > >> > > > > a > > >> > > > > >>>>> async cache operation being followed by some other cache > > >> > > operation > > >> > > > > (an > > >> > > > > >>>>> immediate issue), and more a general case of the > > >> continuation > > >> > of > > >> > > > > >>>>> application logic using a striped pool thread in a way > > that > > >> > might > > >> > > > > mean > > >> > > > > >>>>> that > > >> > > > > >>>>> thread is never given back - it's now just a piece of > the > > >> > > > application > > >> > > > > >>>>> infrastructure until some other application activity > > >> schedules > > >> > > away > > >> > > > > from > > >> > > > > >>>>> that thread (eg: by ContinueWith or some other async > > >> operation > > >> > in > > >> > > > the > > >> > > > > >>>>> application code that releases the thread). To be fair, > > >> beyond > > >> > > > > >>>>> structures > > >> > > > > >>>>> like ContinueWith(), it is not obvious how that > > continuation > > >> > > thread > > >> > > > > >>>>> should > > >> > > > > >>>>> be handed back. This will be the same behaviour for > > >> dedicated > > >> > > > > >>>>> continuation > > >> > > > > >>>>> pools, but with far less risk in the absence of > > >> ContinueWith() > > >> > > > > >>>>> constructs. > > >> > > > > >>>>> > > >> > > > > >>>>> In the .Net world this is becoming more of an issue as > > fewer > > >> > .Net > > >> > > > use > > >> > > > > >>>>> cases > > >> > > > > >>>>> outside of UI bother with synchronization contexts by > > >> default. > > >> > > > > >>>>> > > >> > > > > >>>>> Raymond. > > >> > > > > >>>>> > > >> > > > > >>>>> > > >> > > > > >>>>> On Thu, Mar 18, 2021 at 9:56 AM Valentin Kulichenko < > > >> > > > > >>>>> valentin.kuliche...@gmail.com> wrote: > > >> > > > > >>>>> > > >> > > > > >>>>>> Hi Denis, > > >> > > > > >>>>>> > > >> > > > > >>>>>> I think Pavel's main point is that behavior is > > >> unpredictable. > > >> > > For > > >> > > > > >>>>> example, > > >> > > > > >>>>>> AFAIK, putAsync can be executed in the main thread > > instead > > >> of > > >> > > the > > >> > > > > >>>>> striped > > >> > > > > >>>>>> pool thread if the operation is local. The listener can > > >> also > > >> > be > > >> > > > > >>>>> executed in > > >> > > > > >>>>>> the main thread - this happens if the future is > completed > > >> > prior > > >> > > to > > >> > > > > >>>>> listener > > >> > > > > >>>>>> invocation (this is actually quite possible in the unit > > >> test > > >> > > > > >>>>> environment > > >> > > > > >>>>>> causing the test to pass). Finally, I'm pretty sure > there > > >> are > > >> > > many > > >> > > > > >>>>> cases > > >> > > > > >>>>>> when a deadlock does not occur right away, but instead > it > > >> will > > >> > > > > reveal > > >> > > > > >>>>>> itself under high load due to thread starvation. The > > latter > > >> > type > > >> > > > of > > >> > > > > >>>>> issues > > >> > > > > >>>>>> is the most dangerous because they are often reproduced > > >> only > > >> > in > > >> > > > > >>>>> production. > > >> > > > > >>>>>> Finally, there are performance considerations as well - > > >> cache > > >> > > > > >>>>> operations > > >> > > > > >>>>>> and listeners share the same fixed-sized pool which can > > >> have > > >> > > > > negative > > >> > > > > >>>>>> effects. > > >> > > > > >>>>>> > > >> > > > > >>>>>> I'm OK with the change. Although, it might be better to > > >> > > introduce > > >> > > > a > > >> > > > > >>>>> new > > >> > > > > >>>>>> fixed-sized pool instead of ForkJoinPool for listeners, > > >> simply > > >> > > > > >>>>> because this > > >> > > > > >>>>>> is the approach taken throughout the project. But this > is > > >> up > > >> > to > > >> > > a > > >> > > > > >>>>> debate. > > >> > > > > >>>>>> > > >> > > > > >>>>>> -Val > > >> > > > > >>>>>> > > >> > > > > >>>>>> On Wed, Mar 17, 2021 at 11:31 AM Denis Garus < > > >> > > garus....@gmail.com > > >> > > > > > > >> > > > > >>>>> wrote: > > >> > > > > >>>>>> > > >> > > > > >>>>>>> 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 > > >> > > > > >>>>>>>>>>>>>> > > >> > > > > >>>>>>>>>>>>> > > >> > > > > >>>>>>>>>>>> > > >> > > > > >>>>>>>>>>> > > >> > > > > >>>>>>>>>> > > >> > > > > >>>>>>>>> > > >> > > > > >>>>>>>> > > >> > > > > >>>>>>> > > >> > > > > >>>>>> > > >> > > > > >>>>> > > >> > > > > >>>>> > > >> > > > > >>>>> -- > > >> > > > > >>>>> <http://www.trimble.com/> > > >> > > > > >>>>> Raymond Wilson > > >> > > > > >>>>> Solution Architect, Civil Construction Software Systems > > >> (CCSS) > > >> > > > > >>>>> 11 Birmingham Drive | Christchurch, New Zealand > > >> > > > > >>>>> raymond_wil...@trimble.com > > >> > > > > >>>>> > > >> > > > > >>>>> < > > >> > > > > >>>>> > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch > > >> > > > > >>>>>> > > >> > > > > >>>>> > > >> > > > > >>>> > > >> > > > > > > >> > > > > > >> > > > > >> > > > > >> > > -- > > >> > > > > >> > > Best regards, > > >> > > Alexei Scherbakov > > >> > > > > >> > > > >> > > >> > > >> -- > > >> > > >> Best regards, > > >> Alexei Scherbakov > > >> > > > > > > > > -- > > Best regards, > Alexei Scherbakov >
