You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Pavel Tupitsyn <pt...@apache.org> on 2021/04/12 17:17:51 UTC
Re: IEP-70: Async Continuation Executor
Stan,
Sorry for the late reply (had a vacation).
> In my ideal world, the users don't configure thread pools, they just have
safe default behavior (async execution)
> and a way to make it fast for one particular function (with annotation or
anything else).
I've
added testRemoteOperationListenerExecutesOnStripedPoolWhenCustomExecutorIsProvided
to demonstrate this use case.
> I'm OK to proceed with the approach you're suggesting if I haven't
convinced you by now
Are you OK to merge the changes as is (ForkJoinPool#commonPool as the
default executor),
or do we change it to Ignite public pool?
On Mon, Mar 29, 2021 at 11:09 PM Stanislav Lukyanov <st...@gmail.com>
wrote:
> But what if I need to have exactly one callback synchronous, and all other
> can be asynchronous?
>
> I would separate two cases: an existing user who wants their old behavior
> back, and a new user that wants to fine tune their app.
> The existing user needs a global "make it all synchronous" switch.
> The new user should only enable the fast-but-dangerous behavior locally,
> exactly where they need it.
>
> In my ideal world, the users don't configure thread pools, they just have
> safe default behavior (async execution)
> and a way to make it fast for one particular function (with annotation or
> anything else).
> Also, this should work in a similar way for different APIs - so I'm trying
> to lay some basis to rework all of these continuous queries and event
> listeners,
> even though they're explicitly mentioned as out of scope for IEP-70.
>
> At the same time, I understand that any change we make now will have pros
> and cons, and we can't make it perfect because of compatibility reasons.
> I'm OK to proceed with the approach you're suggesting if I haven't
> convinced you by now :)
>
> Thanks,
> Stan
>
> > On 29 Mar 2021, at 22:47, Pavel Tupitsyn <pt...@apache.org> wrote:
> >
> > Stan,
> >
> > Unfortunately, annotations have a few drawbacks:
> > * Can't configure it globally ("I already use sync callbacks, give me
> back
> > the old behavior in one line")
> > * Can't configure in Spring
> > * Useless in C++ & .NET
> > * You can already specify executor in IgniteFuture#listenAsync, so there
> > seems to be no added value
> >
> >> the only value we really expect the user to set in that property is
> > Runnable::run
> > Not really - there are lots of available options [1].
> > Some apps may already have one or more thread pools that can be used for
> > continuations.
> >
> >> you can't specify Runnable::run in a Spring XML
> > Good point, but creating a class for that is trivial.
> > We can ship a ready-made class and mention it in the docs for simplicity.
> >
> >
> > Globally configurable Executor fits nicely with
> > existing IgniteFuture#listenAsync,
> > not sure why you dislike it.
> >
> >
> > [1]
> >
> https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html
> >
> > On Mon, Mar 29, 2021 at 10:23 PM Stanislav Lukyanov <
> stanlukyanov@gmail.com>
> > wrote:
> >
> >> 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 <pt...@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.scherbakoff@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 <pt...@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 <
> ptupitsyn@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.scherbakoff@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 <ptupitsyn@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.scherbakoff@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 <
> ptupitsyn@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 <st...@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 <
> >>>>> ptupitsyn@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 <
> >>>>>>>>> ptupitsyn@apache.org
> >>>>>>>>>>>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Ready for review:
> >>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
> >>>>>>>>> ptupitsyn@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 <
> >>>>>>>>>> ptupitsyn@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_wilson@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.kulichenko@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.d.g@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.koptilin@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 <
> >>>>>>>>>> ptupitsyn@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.koptilin@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 <
> >>>>>>>>>>>>>>>> ptupitsyn@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.koptilin@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.koptilin@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 <
> >>>>>>>>>>>>>>>>>>> ptupitsyn@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_wilson@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
> >>>>
> >>
> >>
>
>
Re: IEP-70: Async Continuation Executor
Posted by Pavel Tupitsyn <pt...@apache.org>.
I'll keep the common pool then, thank you.
Public pool is the "work-horse of the Compute Grid" [1], so it does not
seem to fit here at all.
[1]
https://ignite.apache.org/docs/latest/perf-and-troubleshooting/thread-pools-tuning#public-pool
On Thu, Apr 15, 2021 at 2:18 PM Stanislav Lukyanov <st...@gmail.com>
wrote:
> Giving this another thought - I'm kinda torn on this myself now, as I like
> you argument that a chain of multiple (GG and not GG) continuations should
> execute in the same pool.
> This would probably be easier to understand for a beginning or
> intermediate Ignite user, which is the majority.
> Anyway, I'll leave to you to decide.
>
> > On 15 Apr 2021, at 11:02, Stanislav Lukyanov <st...@gmail.com>
> wrote:
> >
> > Hi Pavel,
> >
> > I'd prefer public pool.
> >
> > Thanks,
> > Stan
> >
> >> On 12 Apr 2021, at 20:17, Pavel Tupitsyn <pt...@apache.org> wrote:
> >>
> >> Stan,
> >>
> >> Sorry for the late reply (had a vacation).
> >>
> >>> In my ideal world, the users don't configure thread pools, they just
> have
> >> safe default behavior (async execution)
> >>> and a way to make it fast for one particular function (with annotation
> or
> >> anything else).
> >>
> >> I've
> >> added
> testRemoteOperationListenerExecutesOnStripedPoolWhenCustomExecutorIsProvided
> >> to demonstrate this use case.
> >>
> >>
> >>> I'm OK to proceed with the approach you're suggesting if I haven't
> >> convinced you by now
> >>
> >> Are you OK to merge the changes as is (ForkJoinPool#commonPool as the
> >> default executor),
> >> or do we change it to Ignite public pool?
> >>
> >> On Mon, Mar 29, 2021 at 11:09 PM Stanislav Lukyanov <
> stanlukyanov@gmail.com>
> >> wrote:
> >>
> >>> But what if I need to have exactly one callback synchronous, and all
> other
> >>> can be asynchronous?
> >>>
> >>> I would separate two cases: an existing user who wants their old
> behavior
> >>> back, and a new user that wants to fine tune their app.
> >>> The existing user needs a global "make it all synchronous" switch.
> >>> The new user should only enable the fast-but-dangerous behavior
> locally,
> >>> exactly where they need it.
> >>>
> >>> In my ideal world, the users don't configure thread pools, they just
> have
> >>> safe default behavior (async execution)
> >>> and a way to make it fast for one particular function (with annotation
> or
> >>> anything else).
> >>> Also, this should work in a similar way for different APIs - so I'm
> trying
> >>> to lay some basis to rework all of these continuous queries and event
> >>> listeners,
> >>> even though they're explicitly mentioned as out of scope for IEP-70.
> >>>
> >>> At the same time, I understand that any change we make now will have
> pros
> >>> and cons, and we can't make it perfect because of compatibility
> reasons.
> >>> I'm OK to proceed with the approach you're suggesting if I haven't
> >>> convinced you by now :)
> >>>
> >>> Thanks,
> >>> Stan
> >>>
> >>>> On 29 Mar 2021, at 22:47, Pavel Tupitsyn <pt...@apache.org>
> wrote:
> >>>>
> >>>> Stan,
> >>>>
> >>>> Unfortunately, annotations have a few drawbacks:
> >>>> * Can't configure it globally ("I already use sync callbacks, give me
> >>> back
> >>>> the old behavior in one line")
> >>>> * Can't configure in Spring
> >>>> * Useless in C++ & .NET
> >>>> * You can already specify executor in IgniteFuture#listenAsync, so
> there
> >>>> seems to be no added value
> >>>>
> >>>>> the only value we really expect the user to set in that property is
> >>>> Runnable::run
> >>>> Not really - there are lots of available options [1].
> >>>> Some apps may already have one or more thread pools that can be used
> for
> >>>> continuations.
> >>>>
> >>>>> you can't specify Runnable::run in a Spring XML
> >>>> Good point, but creating a class for that is trivial.
> >>>> We can ship a ready-made class and mention it in the docs for
> simplicity.
> >>>>
> >>>>
> >>>> Globally configurable Executor fits nicely with
> >>>> existing IgniteFuture#listenAsync,
> >>>> not sure why you dislike it.
> >>>>
> >>>>
> >>>> [1]
> >>>>
> >>>
> https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html
> >>>>
> >>>> On Mon, Mar 29, 2021 at 10:23 PM Stanislav Lukyanov <
> >>> stanlukyanov@gmail.com>
> >>>> wrote:
> >>>>
> >>>>> 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 <pt...@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.scherbakoff@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 <ptupitsyn@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 <
> >>> ptupitsyn@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.scherbakoff@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 <
> ptupitsyn@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.scherbakoff@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 <
> >>> ptupitsyn@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 <st...@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 <
> >>>>>>>> ptupitsyn@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 <
> >>>>>>>>>>>> ptupitsyn@apache.org
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Ready for review:
> >>>>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
> >>>>>>>>>>>> ptupitsyn@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 <
> >>>>>>>>>>>>> ptupitsyn@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_wilson@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.kulichenko@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.d.g@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.koptilin@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 <
> >>>>>>>>>>>>> ptupitsyn@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.koptilin@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 <
> >>>>>>>>>>>>>>>>>>> ptupitsyn@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.koptilin@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.koptilin@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 <
> >>>>>>>>>>>>>>>>>>>>>> ptupitsyn@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_wilson@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
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >
>
>
Re: IEP-70: Async Continuation Executor
Posted by Stanislav Lukyanov <st...@gmail.com>.
Giving this another thought - I'm kinda torn on this myself now, as I like you argument that a chain of multiple (GG and not GG) continuations should execute in the same pool.
This would probably be easier to understand for a beginning or intermediate Ignite user, which is the majority.
Anyway, I'll leave to you to decide.
> On 15 Apr 2021, at 11:02, Stanislav Lukyanov <st...@gmail.com> wrote:
>
> Hi Pavel,
>
> I'd prefer public pool.
>
> Thanks,
> Stan
>
>> On 12 Apr 2021, at 20:17, Pavel Tupitsyn <pt...@apache.org> wrote:
>>
>> Stan,
>>
>> Sorry for the late reply (had a vacation).
>>
>>> In my ideal world, the users don't configure thread pools, they just have
>> safe default behavior (async execution)
>>> and a way to make it fast for one particular function (with annotation or
>> anything else).
>>
>> I've
>> added testRemoteOperationListenerExecutesOnStripedPoolWhenCustomExecutorIsProvided
>> to demonstrate this use case.
>>
>>
>>> I'm OK to proceed with the approach you're suggesting if I haven't
>> convinced you by now
>>
>> Are you OK to merge the changes as is (ForkJoinPool#commonPool as the
>> default executor),
>> or do we change it to Ignite public pool?
>>
>> On Mon, Mar 29, 2021 at 11:09 PM Stanislav Lukyanov <st...@gmail.com>
>> wrote:
>>
>>> But what if I need to have exactly one callback synchronous, and all other
>>> can be asynchronous?
>>>
>>> I would separate two cases: an existing user who wants their old behavior
>>> back, and a new user that wants to fine tune their app.
>>> The existing user needs a global "make it all synchronous" switch.
>>> The new user should only enable the fast-but-dangerous behavior locally,
>>> exactly where they need it.
>>>
>>> In my ideal world, the users don't configure thread pools, they just have
>>> safe default behavior (async execution)
>>> and a way to make it fast for one particular function (with annotation or
>>> anything else).
>>> Also, this should work in a similar way for different APIs - so I'm trying
>>> to lay some basis to rework all of these continuous queries and event
>>> listeners,
>>> even though they're explicitly mentioned as out of scope for IEP-70.
>>>
>>> At the same time, I understand that any change we make now will have pros
>>> and cons, and we can't make it perfect because of compatibility reasons.
>>> I'm OK to proceed with the approach you're suggesting if I haven't
>>> convinced you by now :)
>>>
>>> Thanks,
>>> Stan
>>>
>>>> On 29 Mar 2021, at 22:47, Pavel Tupitsyn <pt...@apache.org> wrote:
>>>>
>>>> Stan,
>>>>
>>>> Unfortunately, annotations have a few drawbacks:
>>>> * Can't configure it globally ("I already use sync callbacks, give me
>>> back
>>>> the old behavior in one line")
>>>> * Can't configure in Spring
>>>> * Useless in C++ & .NET
>>>> * You can already specify executor in IgniteFuture#listenAsync, so there
>>>> seems to be no added value
>>>>
>>>>> the only value we really expect the user to set in that property is
>>>> Runnable::run
>>>> Not really - there are lots of available options [1].
>>>> Some apps may already have one or more thread pools that can be used for
>>>> continuations.
>>>>
>>>>> you can't specify Runnable::run in a Spring XML
>>>> Good point, but creating a class for that is trivial.
>>>> We can ship a ready-made class and mention it in the docs for simplicity.
>>>>
>>>>
>>>> Globally configurable Executor fits nicely with
>>>> existing IgniteFuture#listenAsync,
>>>> not sure why you dislike it.
>>>>
>>>>
>>>> [1]
>>>>
>>> https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html
>>>>
>>>> On Mon, Mar 29, 2021 at 10:23 PM Stanislav Lukyanov <
>>> stanlukyanov@gmail.com>
>>>> wrote:
>>>>
>>>>> 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 <pt...@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.scherbakoff@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 <pt...@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 <
>>> ptupitsyn@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.scherbakoff@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 <ptupitsyn@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.scherbakoff@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 <
>>> ptupitsyn@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 <st...@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 <
>>>>>>>> ptupitsyn@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 <
>>>>>>>>>>>> ptupitsyn@apache.org
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Ready for review:
>>>>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
>>>>>>>>>>>> ptupitsyn@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 <
>>>>>>>>>>>>> ptupitsyn@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_wilson@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.kulichenko@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.d.g@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.koptilin@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 <
>>>>>>>>>>>>> ptupitsyn@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.koptilin@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 <
>>>>>>>>>>>>>>>>>>> ptupitsyn@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.koptilin@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.koptilin@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 <
>>>>>>>>>>>>>>>>>>>>>> ptupitsyn@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_wilson@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
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
Re: IEP-70: Async Continuation Executor
Posted by Stanislav Lukyanov <st...@gmail.com>.
Hi Pavel,
I'd prefer public pool.
Thanks,
Stan
> On 12 Apr 2021, at 20:17, Pavel Tupitsyn <pt...@apache.org> wrote:
>
> Stan,
>
> Sorry for the late reply (had a vacation).
>
>> In my ideal world, the users don't configure thread pools, they just have
> safe default behavior (async execution)
>> and a way to make it fast for one particular function (with annotation or
> anything else).
>
> I've
> added testRemoteOperationListenerExecutesOnStripedPoolWhenCustomExecutorIsProvided
> to demonstrate this use case.
>
>
>> I'm OK to proceed with the approach you're suggesting if I haven't
> convinced you by now
>
> Are you OK to merge the changes as is (ForkJoinPool#commonPool as the
> default executor),
> or do we change it to Ignite public pool?
>
> On Mon, Mar 29, 2021 at 11:09 PM Stanislav Lukyanov <st...@gmail.com>
> wrote:
>
>> But what if I need to have exactly one callback synchronous, and all other
>> can be asynchronous?
>>
>> I would separate two cases: an existing user who wants their old behavior
>> back, and a new user that wants to fine tune their app.
>> The existing user needs a global "make it all synchronous" switch.
>> The new user should only enable the fast-but-dangerous behavior locally,
>> exactly where they need it.
>>
>> In my ideal world, the users don't configure thread pools, they just have
>> safe default behavior (async execution)
>> and a way to make it fast for one particular function (with annotation or
>> anything else).
>> Also, this should work in a similar way for different APIs - so I'm trying
>> to lay some basis to rework all of these continuous queries and event
>> listeners,
>> even though they're explicitly mentioned as out of scope for IEP-70.
>>
>> At the same time, I understand that any change we make now will have pros
>> and cons, and we can't make it perfect because of compatibility reasons.
>> I'm OK to proceed with the approach you're suggesting if I haven't
>> convinced you by now :)
>>
>> Thanks,
>> Stan
>>
>>> On 29 Mar 2021, at 22:47, Pavel Tupitsyn <pt...@apache.org> wrote:
>>>
>>> Stan,
>>>
>>> Unfortunately, annotations have a few drawbacks:
>>> * Can't configure it globally ("I already use sync callbacks, give me
>> back
>>> the old behavior in one line")
>>> * Can't configure in Spring
>>> * Useless in C++ & .NET
>>> * You can already specify executor in IgniteFuture#listenAsync, so there
>>> seems to be no added value
>>>
>>>> the only value we really expect the user to set in that property is
>>> Runnable::run
>>> Not really - there are lots of available options [1].
>>> Some apps may already have one or more thread pools that can be used for
>>> continuations.
>>>
>>>> you can't specify Runnable::run in a Spring XML
>>> Good point, but creating a class for that is trivial.
>>> We can ship a ready-made class and mention it in the docs for simplicity.
>>>
>>>
>>> Globally configurable Executor fits nicely with
>>> existing IgniteFuture#listenAsync,
>>> not sure why you dislike it.
>>>
>>>
>>> [1]
>>>
>> https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/Executors.html
>>>
>>> On Mon, Mar 29, 2021 at 10:23 PM Stanislav Lukyanov <
>> stanlukyanov@gmail.com>
>>> wrote:
>>>
>>>> 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 <pt...@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.scherbakoff@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 <pt...@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 <
>> ptupitsyn@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.scherbakoff@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 <ptupitsyn@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.scherbakoff@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 <
>> ptupitsyn@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 <st...@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 <
>>>>>>> ptupitsyn@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 <
>>>>>>>>>>> ptupitsyn@apache.org
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ready for review:
>>>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
>>>>>>>>>>> ptupitsyn@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 <
>>>>>>>>>>>> ptupitsyn@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_wilson@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.kulichenko@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.d.g@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.koptilin@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 <
>>>>>>>>>>>> ptupitsyn@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.koptilin@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 <
>>>>>>>>>>>>>>>>>> ptupitsyn@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.koptilin@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.koptilin@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 <
>>>>>>>>>>>>>>>>>>>>> ptupitsyn@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_wilson@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
>>>>>>
>>>>
>>>>
>>
>>