Hi Pavel,
I'd prefer public pool.
Thanks,
Stan
> On 12 Apr 2021, at 20:17, Pavel Tupitsyn <ptupit...@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 <stanlukya...@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 <ptupit...@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 <
>> stanlukya...@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 <ptupit...@apache.org> wrote:
>>>>>
>>>>> Stan,
>>>>>
>>>>> I'm ok with using public pool by default, but we need a way to restore
>>>> the
>>>>> old behavior, do you agree?
>>>>> I think we should keep the new IgniteConfiguration property.
>>>>>
>>>>> On Fri, Mar 26, 2021 at 2:12 PM Alexei Scherbakov <
>>>>> alexey.scherbak...@gmail.com> wrote:
>>>>>
>>>>>> Pavel,
>>>>>>
>>>>>> Dedicated pool looks safer and more manageable to me. Make sure the
>>>> threads
>>>>>> in the pool are lazily started and stopped if not used for some time.
>>>>>>
>>>>>> Because I have no more real arguments against the change, I suggest to
>>>>>> proceed with this approach.
>>>>>>
>>>>>> чт, 25 мар. 2021 г. в 22:16, Pavel Tupitsyn <ptupit...@apache.org>:
>>>>>>
>>>>>>> Alexei,
>>>>>>>
>>>>>>>> we already have ways to control a listener's behavior
>>>>>>> No, we don't have a way to fix current broken and dangerous behavior
>>>>>>> globally.
>>>>>>> You should not expect the user to fix every async call manually.
>>>>>>>
>>>>>>>> commonPool can alter existing deployments in unpredictable ways,
>>>>>>>> if commonPool is heavily used for other purposes
>>>>>>> Common pool resizes dynamically to accommodate the load [1]
>>>>>>> What do you think about Stan's suggestion to use our public pool
>>>> instead?
>>>>>>>
>>>>>>> [1]
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>> https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ForkJoinPool.html
>>>>>>>
>>>>>>> On Thu, Mar 25, 2021 at 10:10 PM Pavel Tupitsyn <
>> ptupit...@apache.org>
>>>>>>> wrote:
>>>>>>>
>>>>>>>>> I don't agree that the code isn't related to Ignite - it is
>> something
>>>>>>>> that the user does via Ignite API
>>>>>>>>
>>>>>>>> This is a misconception. When you write general-purpose async code,
>> it
>>>>>>>> looks like this:
>>>>>>>>
>>>>>>>> myClass.fooAsync()
>>>>>>>> .chain(igniteCache.putAsync)
>>>>>>>> .chain(myClass.barAsync)
>>>>>>>> .chain(...)
>>>>>>>>
>>>>>>>> And so on, you jump from one continuation to another.
>>>>>>>> You don't think about this as "I use Ignite API to run my
>>>>>> continuation",
>>>>>>>> this is just another async call among hundreds of others.
>>>>>>>>
>>>>>>>> And you don't want 1 of 20 libraries that you use to have "special
>>>>>> needs"
>>>>>>>> like Ignite does right now.
>>>>>>>>
>>>>>>>> I know Java is late to the async party and not everyone is used to
>>>> this
>>>>>>>> mindset,
>>>>>>>> but the situation changes, more and more code bases go async all the
>>>>>> way,
>>>>>>>> use CompletionStage everywhere, etc.
>>>>>>>>
>>>>>>>>
>>>>>>>>> If we go with the public pool - no additional options needed.
>>>>>>>>
>>>>>>>> I guess public pool should work.
>>>>>>>> However, I would prefer to keep using commonPool, which is
>> recommended
>>>>>>> for
>>>>>>>> a general purpose like this.
>>>>>>>>
>>>>>>>> On Thu, Mar 25, 2021 at 3:56 PM Alexei Scherbakov <
>>>>>>>> alexey.scherbak...@gmail.com> wrote:
>>>>>>>>
>>>>>>>>> Pavel,
>>>>>>>>>
>>>>>>>>> The change still looks a bit risky to me, because the default
>>>> executor
>>>>>>> is
>>>>>>>>> set to commonPool and can alter existing deployments in
>> unpredictable
>>>>>>>>> ways,
>>>>>>>>> if commonPool is heavily used for other purposes.
>>>>>>>>>
>>>>>>>>> Runnable::run usage is not obvious as well and should be properly
>>>>>>>>> documented as a way to return to old behavior.
>>>>>>>>>
>>>>>>>>> I'm not sure we need it in 2.X for the reasons above - we already
>>>> have
>>>>>>>>> ways
>>>>>>>>> to control a listener's behavior - it's a matter of good
>>>> documentation
>>>>>>> to
>>>>>>>>> me.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> чт, 25 мар. 2021 г. в 15:33, Pavel Tupitsyn <ptupit...@apache.org
>>> :
>>>>>>>>>
>>>>>>>>>> Alexei,
>>>>>>>>>>
>>>>>>>>>>> Sometimes it's more desirable to execute the listener in the same
>>>>>>>>> thread
>>>>>>>>>>> It's up to the user to decide.
>>>>>>>>>>
>>>>>>>>>> Yes, we give users a choice to configure the executor as
>>>>>> Runnable::run
>>>>>>>>> and
>>>>>>>>>> use the same thread if needed.
>>>>>>>>>> However, it should not be the default behavior as explained above
>>>>>> (bad
>>>>>>>>>> usability, unexpected major issues).
>>>>>>>>>>
>>>>>>>>>> On Thu, Mar 25, 2021 at 3:06 PM Alexei Scherbakov <
>>>>>>>>>> alexey.scherbak...@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Pavel,
>>>>>>>>>>>
>>>>>>>>>>> While I understand the issue and overall agree with you, I'm
>>>>>> against
>>>>>>>>> the
>>>>>>>>>>> execution of listeners in separate thread pool by default.
>>>>>>>>>>>
>>>>>>>>>>> Sometimes it's more desirable to execute the listener in the same
>>>>>>>>> thread,
>>>>>>>>>>> for example if it's some lightweight closure.
>>>>>>>>>>>
>>>>>>>>>>> It's up to the user to decide.
>>>>>>>>>>>
>>>>>>>>>>> I think the IgniteFuture.listen method should be properly
>>>>>> documented
>>>>>>>>> to
>>>>>>>>>>> avoid execution of cluster operations or any other potentially
>>>>>>>>> blocking
>>>>>>>>>>> operations inside the listener.
>>>>>>>>>>>
>>>>>>>>>>> Otherwise listenAsync should be used.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> чт, 25 мар. 2021 г. в 14:04, Pavel Tupitsyn <
>> ptupit...@apache.org
>>>>>>> :
>>>>>>>>>>>
>>>>>>>>>>>> Stan,
>>>>>>>>>>>>
>>>>>>>>>>>> We have thread pools dedicated for specific purposes, like cache
>>>>>>>>>>> (striped),
>>>>>>>>>>>> compute (pub), query, etc
>>>>>>>>>>>> As I understand it, the reason here is to limit the number of
>>>>>>>>> threads
>>>>>>>>>>>> dedicated to a given subsystem.
>>>>>>>>>>>> For example, Compute may be overloaded with work, but Cache and
>>>>>>>>>> Discovery
>>>>>>>>>>>> will keep going.
>>>>>>>>>>>>
>>>>>>>>>>>> This is different from async continuations, which are arbitrary
>>>>>>> user
>>>>>>>>>>> code.
>>>>>>>>>>>> So what is the benefit of having a new user pool for arbitrary
>>>>>>> code
>>>>>>>>>> that
>>>>>>>>>>> is
>>>>>>>>>>>> probably not related to Ignite at all?
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Mar 25, 2021 at 1:31 PM <stanlukya...@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> Pavel,
>>>>>>>>>>>>>
>>>>>>>>>>>>> This is a great work, fully agree with the overall idea and
>>>>>>>>> approach.
>>>>>>>>>>>>>
>>>>>>>>>>>>> However, I have some reservations about the API. We sure do
>>>>>>> have a
>>>>>>>>>> lot
>>>>>>>>>>> of
>>>>>>>>>>>>> async stuff in the system, and I would suggest to stick to the
>>>>>>>>> usual
>>>>>>>>>>>> design
>>>>>>>>>>>>> - create a separate thread pool, add a single property to
>>>>>>> control
>>>>>>>>> the
>>>>>>>>>>>> size
>>>>>>>>>>>>> of the pool.
>>>>>>>>>>>>> Alternatively, we may consider using public pool for that.
>>>>>> May I
>>>>>>>>> ask
>>>>>>>>>> if
>>>>>>>>>>>>> there is an example use case which doesn’t work with public
>>>>>>> pool?
>>>>>>>>>>>>>
>>>>>>>>>>>>> For .NET, agree that we should follow the rules and APIs of
>>>>>> the
>>>>>>>>>>> platform,
>>>>>>>>>>>>> so the behavior might slightly differ.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>> Stan
>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 24 Mar 2021, at 09:52, Pavel Tupitsyn <
>>>>>>> ptupit...@apache.org>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Igniters, since there are no more comments and/or review
>>>>>>>>> feedback,
>>>>>>>>>>>>>> I'm going to merge the changes by the end of the week.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Mon, Mar 22, 2021 at 10:37 PM Pavel Tupitsyn <
>>>>>>>>>>> ptupit...@apache.org
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ready for review:
>>>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
>>>>>>>>>>> ptupit...@apache.org>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Simple benchmark added - see JmhCacheAsyncListenBenchmark
>>>>>> in
>>>>>>>>> the
>>>>>>>>>>> PR.
>>>>>>>>>>>>>>>> There is a 6-8% drop (1 client, 2 servers, 1 machine, int
>>>>>>>>>> key/val).
>>>>>>>>>>>>>>>> I expect this difference to become barely observable on
>>>>>>>>>> real-world
>>>>>>>>>>>>>>>> workloads.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, Mar 18, 2021 at 12:35 PM Pavel Tupitsyn <
>>>>>>>>>>>> ptupit...@apache.org>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Denis,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> For a reproducer, please see
>>>>>>>>>>> CacheAsyncContinuationExecutorTest.java
>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>>> the linked PoC [1]
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>> https://github.com/apache/ignite/pull/8870/files#diff-c788c12013622093df07d8f628a6e8c5fb0c15007f0787f2d459dbb3e377fc5aR54
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On Thu, Mar 18, 2021 at 1:58 AM Raymond Wilson <
>>>>>>>>>>>>>>>>> raymond_wil...@trimble.com> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> We implemented the ContinueWith() suggestion from Pavel,
>>>>>>>>> and it
>>>>>>>>>>>> works
>>>>>>>>>>>>>>>>>> well
>>>>>>>>>>>>>>>>>> so far in testing, though we do not have data to support
>>>>>>> if
>>>>>>>>>> there
>>>>>>>>>>>> is
>>>>>>>>>>>>> or
>>>>>>>>>>>>>>>>>> is
>>>>>>>>>>>>>>>>>> not a performance penalty in our use case..
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> To lend another vote to the 'Don't do continuations on
>>>>>> the
>>>>>>>>>>> striped
>>>>>>>>>>>>>>>>>> thread
>>>>>>>>>>>>>>>>>> pool' line of thinking: Deadlocking is an issue as is
>>>>>>>>>> starvation.
>>>>>>>>>>>> In
>>>>>>>>>>>>>>>>>> some
>>>>>>>>>>>>>>>>>> ways starvation is more insidious because by the time
>>>>>>> things
>>>>>>>>>> stop
>>>>>>>>>>>>>>>>>> working
>>>>>>>>>>>>>>>>>> the cause and effect distance may be large.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I appreciate the documentation does make statements
>>>>>> about
>>>>>>>>> not
>>>>>>>>>>>>> performing
>>>>>>>>>>>>>>>>>> cache operations in a continuation due to deadlock
>>>>>>>>>> possibilities,
>>>>>>>>>>>> but
>>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>>> statement does not reveal why this is an issue. It is
>>>>>>> less a
>>>>>>>>>> case
>>>>>>>>>>>> of
>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>> async cache operation being followed by some other cache
>>>>>>>>>>> operation
>>>>>>>>>>>>> (an
>>>>>>>>>>>>>>>>>> immediate issue), and more a general case of the
>>>>>>>>> continuation
>>>>>>>>>> of
>>>>>>>>>>>>>>>>>> application logic using a striped pool thread in a way
>>>>>>> that
>>>>>>>>>> might
>>>>>>>>>>>>> mean
>>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>>> thread is never given back - it's now just a piece of
>>>>>> the
>>>>>>>>>>>> application
>>>>>>>>>>>>>>>>>> infrastructure until some other application activity
>>>>>>>>> schedules
>>>>>>>>>>> away
>>>>>>>>>>>>> from
>>>>>>>>>>>>>>>>>> that thread (eg: by ContinueWith or some other async
>>>>>>>>> operation
>>>>>>>>>> in
>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>> application code that releases the thread). To be fair,
>>>>>>>>> beyond
>>>>>>>>>>>>>>>>>> structures
>>>>>>>>>>>>>>>>>> like ContinueWith(), it is not obvious how that
>>>>>>> continuation
>>>>>>>>>>> thread
>>>>>>>>>>>>>>>>>> should
>>>>>>>>>>>>>>>>>> be handed back. This will be the same behaviour for
>>>>>>>>> dedicated
>>>>>>>>>>>>>>>>>> continuation
>>>>>>>>>>>>>>>>>> pools, but with far less risk in the absence of
>>>>>>>>> ContinueWith()
>>>>>>>>>>>>>>>>>> constructs.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> In the .Net world this is becoming more of an issue as
>>>>>>> fewer
>>>>>>>>>> .Net
>>>>>>>>>>>> use
>>>>>>>>>>>>>>>>>> cases
>>>>>>>>>>>>>>>>>> outside of UI bother with synchronization contexts by
>>>>>>>>> default.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Raymond.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On Thu, Mar 18, 2021 at 9:56 AM Valentin Kulichenko <
>>>>>>>>>>>>>>>>>> valentin.kuliche...@gmail.com> wrote:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Hi Denis,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I think Pavel's main point is that behavior is
>>>>>>>>> unpredictable.
>>>>>>>>>>> For
>>>>>>>>>>>>>>>>>> example,
>>>>>>>>>>>>>>>>>>> AFAIK, putAsync can be executed in the main thread
>>>>>>> instead
>>>>>>>>> of
>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>> striped
>>>>>>>>>>>>>>>>>>> pool thread if the operation is local. The listener can
>>>>>>>>> also
>>>>>>>>>> be
>>>>>>>>>>>>>>>>>> executed in
>>>>>>>>>>>>>>>>>>> the main thread - this happens if the future is
>>>>>> completed
>>>>>>>>>> prior
>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>> listener
>>>>>>>>>>>>>>>>>>> invocation (this is actually quite possible in the unit
>>>>>>>>> test
>>>>>>>>>>>>>>>>>> environment
>>>>>>>>>>>>>>>>>>> causing the test to pass). Finally, I'm pretty sure
>>>>>> there
>>>>>>>>> are
>>>>>>>>>>> many
>>>>>>>>>>>>>>>>>> cases
>>>>>>>>>>>>>>>>>>> when a deadlock does not occur right away, but instead
>>>>>> it
>>>>>>>>> will
>>>>>>>>>>>>> reveal
>>>>>>>>>>>>>>>>>>> itself under high load due to thread starvation. The
>>>>>>> latter
>>>>>>>>>> type
>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>>> issues
>>>>>>>>>>>>>>>>>>> is the most dangerous because they are often reproduced
>>>>>>>>> only
>>>>>>>>>> in
>>>>>>>>>>>>>>>>>> production.
>>>>>>>>>>>>>>>>>>> Finally, there are performance considerations as well -
>>>>>>>>> cache
>>>>>>>>>>>>>>>>>> operations
>>>>>>>>>>>>>>>>>>> and listeners share the same fixed-sized pool which can
>>>>>>>>> have
>>>>>>>>>>>>> negative
>>>>>>>>>>>>>>>>>>> effects.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I'm OK with the change. Although, it might be better to
>>>>>>>>>>> introduce
>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>> new
>>>>>>>>>>>>>>>>>>> fixed-sized pool instead of ForkJoinPool for listeners,
>>>>>>>>> simply
>>>>>>>>>>>>>>>>>> because this
>>>>>>>>>>>>>>>>>>> is the approach taken throughout the project. But this
>>>>>> is
>>>>>>>>> up
>>>>>>>>>> to
>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>> debate.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> -Val
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On Wed, Mar 17, 2021 at 11:31 AM Denis Garus <
>>>>>>>>>>> garus....@gmail.com
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Pavel,
>>>>>>>>>>>>>>>>>>>> I tried this:
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> @Test
>>>>>>>>>>>>>>>>>>>> public void test() throws Exception {
>>>>>>>>>>>>>>>>>>>> IgniteCache<Integer, String> cache =
>>>>>>>>>>>>>>>>>>>> startGrid().getOrCreateCache("test_cache");
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> cache.putAsync(1, "one").listen(f ->
>>>>>> cache.replace(1,
>>>>>>>>>>> "two"));
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> assertEquals("two", cache.get(1));
>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> and this test is green.
>>>>>>>>>>>>>>>>>>>> I believe that an user can make listener that leads to
>>>>>>>>>>> deadlock,
>>>>>>>>>>>>> but
>>>>>>>>>>>>>>>>>>>> the example in the IEP does not reflect this.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> ср, 17 мар. 2021 г. в 17:36, Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>> slava.kopti...@gmail.com
>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Hi Pavel,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Not a good excuse really. We have a usability
>>>>>> problem,
>>>>>>>>> you
>>>>>>>>>>> have
>>>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>> admit
>>>>>>>>>>>>>>>>>>>>> it.
>>>>>>>>>>>>>>>>>>>>> Fair enough. I agree that this is a usability issue,
>>>>>>> but
>>>>>>>>> I
>>>>>>>>>>> have
>>>>>>>>>>>>>>>>>> doubts
>>>>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>>>>>> the proposed approach to overcome it is the best one.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Documentation won't help - no one is going to read
>>>>>> the
>>>>>>>>>>> Javadoc
>>>>>>>>>>>>>>>>>> for a
>>>>>>>>>>>>>>>>>>>>> trivial method like putAsync
>>>>>>>>>>>>>>>>>>>>> That is sad... However, I don't think that this is a
>>>>>>>>> strong
>>>>>>>>>>>>>>>>>> argument
>>>>>>>>>>>>>>>>>>>> here.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> This is just my opinion. Let's see what other
>>>>>> community
>>>>>>>>>>> members
>>>>>>>>>>>>>>>>>> have to
>>>>>>>>>>>>>>>>>>>>> say.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>> S.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> ср, 17 мар. 2021 г. в 17:01, Pavel Tupitsyn <
>>>>>>>>>>>> ptupit...@apache.org
>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> the user should use the right API
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Not a good excuse really. We have a usability
>>>>>> problem,
>>>>>>>>> you
>>>>>>>>>>> have
>>>>>>>>>>>>>>>>>> to
>>>>>>>>>>>>>>>>>>>> admit
>>>>>>>>>>>>>>>>>>>>>> it.
>>>>>>>>>>>>>>>>>>>>>> "The brakes did not work on your car - too bad, you
>>>>>>>>> should
>>>>>>>>>>> have
>>>>>>>>>>>>>>>>>> known
>>>>>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>>>>>>> on Sundays only your left foot is allowed on the
>>>>>>> pedal"
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> This particular use case is too intricate.
>>>>>>>>>>>>>>>>>>>>>> Even when you know about that, it is difficult to
>>>>>>> decide
>>>>>>>>>> what
>>>>>>>>>>>>>>>>>> can run
>>>>>>>>>>>>>>>>>>>> on
>>>>>>>>>>>>>>>>>>>>>> the striped pool,
>>>>>>>>>>>>>>>>>>>>>> and what can't. It is too easy to forget.
>>>>>>>>>>>>>>>>>>>>>> And most people don't know, even among Ignite
>>>>>>>>> developers.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Documentation won't help - no one is going to read
>>>>>> the
>>>>>>>>>>> Javadoc
>>>>>>>>>>>>>>>>>> for a
>>>>>>>>>>>>>>>>>>>>>> trivial method like putAsync.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> So I propose to have a safe default.
>>>>>>>>>>>>>>>>>>>>>> Then document the performance tuning opportunity on
>>>>>>> [1].
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Think about how many users abandon a product because
>>>>>>> it
>>>>>>>>>>>>>>>>>> mysteriously
>>>>>>>>>>>>>>>>>>>>>> crashes and hangs.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>> https://ignite.apache.org/docs/latest/perf-and-troubleshooting/general-perf-tips
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> On Wed, Mar 17, 2021 at 4:21 PM Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>>>> slava.kopti...@gmail.com>
>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Hi Pavel,
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Well, I think that the user should use the right
>>>>>> API
>>>>>>>>>> instead
>>>>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>>>>>>> introducing
>>>>>>>>>>>>>>>>>>>>>>> uncontested overhead for everyone.
>>>>>>>>>>>>>>>>>>>>>>> For instance, the code that is provided by IEP can
>>>>>>>>> changed
>>>>>>>>>>> as
>>>>>>>>>>>>>>>>>>>> follows:
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> IgniteFuture fut = cache.putAsync(1, 1);
>>>>>>>>>>>>>>>>>>>>>>> fut.listenAync(f -> {
>>>>>>>>>>>>>>>>>>>>>>> // Executes on Striped pool and deadlocks.
>>>>>>>>>>>>>>>>>>>>>>> cache.replace(1, 2);
>>>>>>>>>>>>>>>>>>>>>>> }, ForkJoinPool.commonPool());
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Of course, it does not mean that this fact should
>>>>>> not
>>>>>>>>> be
>>>>>>>>>>>>>>>>>> properly
>>>>>>>>>>>>>>>>>>>>>>> documented.
>>>>>>>>>>>>>>>>>>>>>>> Perhaps, I am missing something.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>> S.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> ср, 17 мар. 2021 г. в 16:01, Pavel Tupitsyn <
>>>>>>>>>>>>>>>>>> ptupit...@apache.org
>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Slava,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Your suggestion is to keep things as is and
>>>>>> discard
>>>>>>>>> the
>>>>>>>>>>> IEP,
>>>>>>>>>>>>>>>>>>> right?
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> this can lead to significant overhead
>>>>>>>>>>>>>>>>>>>>>>>> Yes, there is some overhead, but the cost of
>>>>>>>>> accidentally
>>>>>>>>>>>>>>>>>>> starving
>>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>> striped pool is worse,
>>>>>>>>>>>>>>>>>>>>>>>> not to mention the deadlocks.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I believe that we should favor correctness over
>>>>>>>>>> performance
>>>>>>>>>>>>>>>>>> in
>>>>>>>>>>>>>>>>>>> any
>>>>>>>>>>>>>>>>>>>>>> case.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> On Wed, Mar 17, 2021 at 3:34 PM Вячеслав Коптилин
>>>>>> <
>>>>>>>>>>>>>>>>>>>>>>>> slava.kopti...@gmail.com>
>>>>>>>>>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Well, the specified method already exists :)
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> /**
>>>>>>>>>>>>>>>>>>>>>>>>> * Registers listener closure to be
>>>>>>> asynchronously
>>>>>>>>>>>>>>>>>> notified
>>>>>>>>>>>>>>>>>>>>>>> whenever
>>>>>>>>>>>>>>>>>>>>>>>>> future completes.
>>>>>>>>>>>>>>>>>>>>>>>>> * Closure will be processed in specified
>>>>>>>>> executor.
>>>>>>>>>>>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>>>>>>>>>>>>> * @param lsnr Listener closure to register.
>>>>>>>>> Cannot
>>>>>>>>>> be
>>>>>>>>>>>>>>>>>>> {@code
>>>>>>>>>>>>>>>>>>>>>>> null}.
>>>>>>>>>>>>>>>>>>>>>>>>> * @param exec Executor to run listener.
>>>>>> Cannot
>>>>>>> be
>>>>>>>>>>>>>>>>>> {@code
>>>>>>>>>>>>>>>>>>>>> null}.
>>>>>>>>>>>>>>>>>>>>>>>>> */
>>>>>>>>>>>>>>>>>>>>>>>>> public void listenAsync(IgniteInClosure<?
>>>>>> super
>>>>>>>>>>>>>>>>>>>>> IgniteFuture<V>>
>>>>>>>>>>>>>>>>>>>>>>>> lsnr,
>>>>>>>>>>>>>>>>>>>>>>>>> Executor exec);
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>> S.
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> ср, 17 мар. 2021 г. в 15:25, Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>>>>> slava.kopti...@gmail.com
>>>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Hello Pavel,
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> I took a look at your IEP and pool request. I
>>>>>> have
>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>> following
>>>>>>>>>>>>>>>>>>>>>>>>> concerns.
>>>>>>>>>>>>>>>>>>>>>>>>>> First of all, this change breaks the contract of
>>>>>>>>>>>>>>>>>>>>>>>>> IgniteFuture#listen(lsnr)
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> /**
>>>>>>>>>>>>>>>>>>>>>>>>>> * Registers listener closure to be
>>>>>>>>> asynchronously
>>>>>>>>>>>>>>>>>>> notified
>>>>>>>>>>>>>>>>>>>>>>>> whenever
>>>>>>>>>>>>>>>>>>>>>>>>>> future completes.
>>>>>>>>>>>>>>>>>>>>>>>>>> * Closure will be processed in thread that
>>>>>>>>>>>>>>>>>> completes
>>>>>>>>>>>>>>>>>>> this
>>>>>>>>>>>>>>>>>>>>>> future
>>>>>>>>>>>>>>>>>>>>>>>> or
>>>>>>>>>>>>>>>>>>>>>>>>>> (if future already
>>>>>>>>>>>>>>>>>>>>>>>>>> * completed) immediately in current thread.
>>>>>>>>>>>>>>>>>>>>>>>>>> *
>>>>>>>>>>>>>>>>>>>>>>>>>> * @param lsnr Listener closure to register.
>>>>>>>>> Cannot
>>>>>>>>>>>>>>>>>> be
>>>>>>>>>>>>>>>>>>>> {@code
>>>>>>>>>>>>>>>>>>>>>>>> null}.
>>>>>>>>>>>>>>>>>>>>>>>>>> */
>>>>>>>>>>>>>>>>>>>>>>>>>> public void listen(IgniteInClosure<? super
>>>>>>>>>>>>>>>>>>> IgniteFuture<V>>
>>>>>>>>>>>>>>>>>>>>>>> lsnr);
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> In your pull request, the listener is always
>>>>>>>>> called
>>>>>>>>>>>>>>>>>> from
>>>>>>>>>>>>>>>>>>> a
>>>>>>>>>>>>>>>>>>>>>>>> specified
>>>>>>>>>>>>>>>>>>>>>>>>>> thread pool (which is fork-join by default)
>>>>>>>>>>>>>>>>>>>>>>>>>> even though the future is already completed
>>>>>> at
>>>>>>>>> the
>>>>>>>>>>>>>>>>>> moment
>>>>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>>>>>>>>> listen
>>>>>>>>>>>>>>>>>>>>>>>>>> method is called.
>>>>>>>>>>>>>>>>>>>>>>>>>> In my opinion, this can lead to significant
>>>>>>>>>>>>>>>>>> overhead -
>>>>>>>>>>>>>>>>>>>>>> submission
>>>>>>>>>>>>>>>>>>>>>>>>>> requires acquiring a lock and notifying a pool
>>>>>>>>> thread.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> It seems to me, that we should not change the
>>>>>>>>>>>>>>>>>> current
>>>>>>>>>>>>>>>>>>>>> behavior.
>>>>>>>>>>>>>>>>>>>>>>>>>> However, thread pool executor can be added as an
>>>>>>>>>>>>>>>>>> optional
>>>>>>>>>>>>>>>>>>>>> parameter
>>>>>>>>>>>>>>>>>>>>>>> of
>>>>>>>>>>>>>>>>>>>>>>>>>> listen() method as follows:
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> public void listen(IgniteInClosure<?
>>>>>> super
>>>>>>>>>>>>>>>>>>>>> IgniteFuture<V>>
>>>>>>>>>>>>>>>>>>>>>>>> lsnr,
>>>>>>>>>>>>>>>>>>>>>>>>>> Executor exec);
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>>> S.
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> пн, 15 мар. 2021 г. в 19:24, Pavel Tupitsyn <
>>>>>>>>>>>>>>>>>>>>> ptupit...@apache.org
>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Igniters,
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> Please review the IEP [1] and let me know your
>>>>>>>>>>>>>>>>>> thoughts.
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>> https://cwiki.apache.org/confluence/display/IGNITE/IEP-70%3A+Async+Continuation+Executor
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> --
>>>>>>>>>>>>>>>>>> <http://www.trimble.com/>
>>>>>>>>>>>>>>>>>> Raymond Wilson
>>>>>>>>>>>>>>>>>> Solution Architect, Civil Construction Software Systems
>>>>>>>>> (CCSS)
>>>>>>>>>>>>>>>>>> 11 Birmingham Drive | Christchurch, New Zealand
>>>>>>>>>>>>>>>>>> raymond_wil...@trimble.com
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> <
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>> https://worksos.trimble.com/?utm_source=Trimble&utm_medium=emailsign&utm_campaign=Launch
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>>
>>>>>>>>>>> Best regards,
>>>>>>>>>>> Alexei Scherbakov
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Alexei Scherbakov
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>>
>>>>>> Best regards,
>>>>>> Alexei Scherbakov
>>>>>>
>>>>
>>>>
>>
>>
