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