Thought about this some more. I agree that we need to be able to switch to synchronous listeners when it's critical for performance. However, I don't like to introduce an Executor property for that. In fact, the only value we really expect the user to set in that property is Runnable::run - seems to be an overkill to have accept an Executor for that. Furthermore, you can't specify Runnable::run in a Spring XML, can you?
I'm thinking that maybe we should go the annotation route here. Let's introduce an annotation @IgniteSyncCallback. It's the same as @IgniteAsyncCallback but reverse :) If a listener is annotated like that then we execute it in the same thread; by default, we execute in the public pool. We can also reuse the same annotation for all other callbacks we have in the system - right now, the callbacks are a mix of sync and async behavior, and we could transition all APIs to use async by default and enforce sync callbacks when the annotation is used. @IgniteAsyncCallback should eventually be deprecated. WDYT? Thanks, Stan > On 29 Mar 2021, at 14:09, Pavel Tupitsyn <ptupit...@apache.org> wrote: > > 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 >>
