IEP-70: Async Continuation Executor

[Pavel Tupitsyn]

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 <[hidden email]>
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 <[hidden email]> 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 <
> > [hidden email]> 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 <[hidden email]>:
> >>
> >>> 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 <[hidden email]>
> >>> 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 <
> >>>> [hidden email]> 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 <[hidden email]>:
> >>>>>
> >>>>>> 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 <
> >>>>>> [hidden email]> 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 <[hidden email]
> >>> :
> >>>>>>>
> >>>>>>>> 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 <[hidden email]> 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 <
> >>> [hidden email]>
> >>>>>>> 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 <
> >>>>>>> [hidden email]
> >>>>>>>>>
> >>>>>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>> Ready for review:
> >>>>>>>>>>> https://github.com/apache/ignite/pull/8870
> >>>>>>>>>>>
> >>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
> >>>>>>> [hidden email]>
> >>>>>>>>>>> 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 <
> >>>>>>>> [hidden email]>
> >>>>>>>>>>>> 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 <
> >>>>>>>>>>>>> [hidden email]> 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 <
> >>>>>>>>>>>>>> [hidden email]> 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 <
> >>>>>>> [hidden email]
> >>>>>>>>>
> >>>>>>>>>>>>>> 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, Вячеслав Коптилин <
> >>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>> 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 <
> >>>>>>>> [hidden email]
> >>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин <
> >>>>>>>>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин
> >> <
> >>>>>>>>>>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
> >>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>> 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
> >>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>> <
> >>>>>>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>
> >>
> 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
> >>
>
>

[Stanislav Lukyanov]

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 <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 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 <
>>> [hidden email]> 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 <[hidden email]>:
>>>>
>>>>> 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 <[hidden email]>
>>>>> 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 <
>>>>>> [hidden email]> 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 <[hidden email]>:
>>>>>>>
>>>>>>>> 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 <
>>>>>>>> [hidden email]> 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 <[hidden email]
>>>>> :
>>>>>>>>>
>>>>>>>>>> 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 <[hidden email]> 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 <
>>>>> [hidden email]>
>>>>>>>>> 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 <
>>>>>>>>> [hidden email]
>>>>>>>>>>>
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Ready for review:
>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
>>>>>>>>> [hidden email]>
>>>>>>>>>>>>> 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 <
>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>> [hidden email]> 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 <
>>>>>>>>>>>>>>>> [hidden email]> 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 <
>>>>>>>>> [hidden email]
>>>>>>>>>>>
>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин
>>>> <
>>>>>>>>>>>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> <
>>>>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>> 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
>>>>
>>
>>

[Pavel Tupitsyn]

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 <[hidden email]>
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 <[hidden email]> 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 <
> [hidden email]>
> > 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 <[hidden email]> 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 <
> >>> [hidden email]> 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 <[hidden email]>:
> >>>>
> >>>>> 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 <
> [hidden email]>
> >>>>> 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 <
> >>>>>> [hidden email]> 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 <[hidden email]
> >:
> >>>>>>>
> >>>>>>>> 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 <
> >>>>>>>> [hidden email]> 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 <
> [hidden email]
> >>>>> :
> >>>>>>>>>
> >>>>>>>>>> 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 <[hidden email]> 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 <
> >>>>> [hidden email]>
> >>>>>>>>> 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 <
> >>>>>>>>> [hidden email]
> >>>>>>>>>>>
> >>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Ready for review:
> >>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
> >>>>>>>>> [hidden email]>
> >>>>>>>>>>>>> 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 <
> >>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>>> [hidden email]> 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 <
> >>>>>>>>>>>>>>>> [hidden email]> 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 <
> >>>>>>>>> [hidden email]
> >>>>>>>>>>>
> >>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
> >>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин <
> >>>>>>>>>>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин
> >>>> <
> >>>>>>>>>>>>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
> >>>>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> 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
> >>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> <
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>>
> >>
> 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
> >>>>
> >>
> >>
>
>

[Stanislav Lukyanov]

Hi Pavel,

I'd prefer public pool.

Thanks,
Stan

> On 12 Apr 2021, at 20:17, Pavel Tupitsyn <[hidden email]> 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 <[hidden email]>
> 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 <[hidden email]> 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 <
>> [hidden email]>
>>> 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 <[hidden email]> 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 <
>>>>> [hidden email]> 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 <[hidden email]>:
>>>>>>
>>>>>>> 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 <
>> [hidden email]>
>>>>>>> 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 <
>>>>>>>> [hidden email]> 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 <[hidden email]
>>> :
>>>>>>>>>
>>>>>>>>>> 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 <
>>>>>>>>>> [hidden email]> 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 <
>> [hidden email]
>>>>>>> :
>>>>>>>>>>>
>>>>>>>>>>>> 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 <[hidden email]> 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 <
>>>>>>> [hidden email]>
>>>>>>>>>>> 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 <
>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>
>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Ready for review:
>>>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>>> [hidden email]> 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 <
>>>>>>>>>>>>>>>>>> [hidden email]> 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 <
>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин
>>>>>> <
>>>>>>>>>>>>>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> <
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>> 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
>>>>>>
>>>>
>>>>
>>
>>

[Stanislav Lukyanov]

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 <[hidden email]> wrote:
>
> Hi Pavel,
>
> I'd prefer public pool.
>
> Thanks,
> Stan
>
>> On 12 Apr 2021, at 20:17, Pavel Tupitsyn <[hidden email]> 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 <[hidden email]>
>> 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 <[hidden email]> 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 <
>>> [hidden email]>
>>>> 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 <[hidden email]> 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 <
>>>>>> [hidden email]> 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 <[hidden email]>:
>>>>>>>
>>>>>>>> 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 <
>>> [hidden email]>
>>>>>>>> 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 <
>>>>>>>>> [hidden email]> 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 <[hidden email]
>>>> :
>>>>>>>>>>
>>>>>>>>>>> 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 <
>>>>>>>>>>> [hidden email]> 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 <
>>> [hidden email]
>>>>>>>> :
>>>>>>>>>>>>
>>>>>>>>>>>>> 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 <[hidden email]> 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 <
>>>>>>>> [hidden email]>
>>>>>>>>>>>> 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 <
>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Ready for review:
>>>>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>>>> [hidden email]> 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 <
>>>>>>>>>>>>>>>>>>> [hidden email]> 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 <
>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин
>>>>>>> <
>>>>>>>>>>>>>>>>>>>>>>>>> [hidden email]>
>>>>>>>>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
>>>>>>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>> 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 <
>>>>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>>>>>> :
>>>>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>>>>> 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
>>>>>>>>>>>>>>>>>>> [hidden email]
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> <
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>> 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
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>

[Pavel Tupitsyn]

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 <[hidden email]>
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 <[hidden email]>
> wrote:
> >
> > Hi Pavel,
> >
> > I'd prefer public pool.
> >
> > Thanks,
> > Stan
> >
> >> On 12 Apr 2021, at 20:17, Pavel Tupitsyn <[hidden email]> 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 <
> [hidden email]>
> >> 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 <[hidden email]>
> 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 <
> >>> [hidden email]>
> >>>> 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 <[hidden email]>
> 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 <
> >>>>>> [hidden email]> 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 <[hidden email]
> >:
> >>>>>>>
> >>>>>>>> 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 <
> >>> [hidden email]>
> >>>>>>>> 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 <
> >>>>>>>>> [hidden email]> 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 <
> [hidden email]
> >>>> :
> >>>>>>>>>>
> >>>>>>>>>>> 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 <
> >>>>>>>>>>> [hidden email]> 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 <
> >>> [hidden email]
> >>>>>>>> :
> >>>>>>>>>>>>
> >>>>>>>>>>>>> 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 <[hidden email]>
> 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 <
> >>>>>>>> [hidden email]>
> >>>>>>>>>>>> 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 <
> >>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> wrote:
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> Ready for review:
> >>>>>>>>>>>>>>>> https://github.com/apache/ignite/pull/8870
> >>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>> On Sun, Mar 21, 2021 at 8:09 PM Pavel Tupitsyn <
> >>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>>>>>> [hidden email]> 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 <
> >>>>>>>>>>>>>>>>>>> [hidden email]> 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 <
> >>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
> >>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин <
> >>>>>>>>>>>>>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>> 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 Вячеслав Коптилин
> >>>>>>> <
> >>>>>>>>>>>>>>>>>>>>>>>>> [hidden email]>
> >>>>>>>>>>>>>>>>>>>>>>>>> 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, Вячеслав Коптилин <
> >>>>>>>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>> 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 <
> >>>>>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>>>>>> :
> >>>>>>>>>>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>>>>>>>>>>> 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
> >>>>>>>>>>>>>>>>>>> [hidden email]
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>>>>>> <
> >>>>>>>>>>>>>>>>>>>
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>
> >>>
> 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
> >>>>>>>
> >>>>>
> >>>>>
> >>>
> >>>
> >
>
>