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