Re[4]: [DISCUSSION] Request for thread unsafe Compute functionality deprecation.

[Zhenya Stanilovsky <ar...@mail.ru.INVALID>]

As previously agreed, ticket [1], examples of concurrent tests you can find here GridDifferentLocalDeploymentSelfTest and here IgniteExplicitImplicitDeploymentSelfTest , TC in progress. Fabric — possibly i mention incorrect word here, more clear it need to be — factory )
I suppose additional tests would be helpful. 
 
thanks !
 
[1]  https://issues.apache.org/jira/browse/IGNITE-14131
>Zhenya,
>
>Could you clarify what you mean by "one instance is shared between numerous
>of fabric"? What is the exact scenario and what are the implications of
>running multiple compute tasks in parallel? A code sample demonstrating the
>scenario might be helpful as well.
>
>-Val
>
>On Fri, Feb 5, 2021 at 12:58 PM Vladislav Pyatkov < vldpyatkov@gmail.com >
>wrote:
> 
>> Hi Zhenya,
>>
>> I don't understand your proposal without a patch.
>> I see that you want to mark as @Depricate three methods in IgniteCompute
>> interface, but what will you give instead of?
>>
>> It seems to me, this interface can invoke some job without contains a class
>> locally.
>> How will this be supported in your mind?
>>
>> On Thu, Jan 28, 2021 at 6:58 PM Ilya Kasnacheev < ilya.kasnacheev@gmail.com
>> >
>> wrote:
>>
>> > Hello!
>> >
>> > Please publish it. I don't see why not.
>> >
>> > Regards,
>> > --
>> > Ilya Kasnacheev
>> >
>> >
>> > чт, 28 янв. 2021 г. в 14:28, Zhenya Stanilovsky
>> < arzamas123@mail.ru.invalid
>> > >:
>> >
>> > >
>> > >
>> > > Hi Ilya , of course it contains in my PR (i don`t know if it shout be
>> > > published before this discussion will be finished).
>> > > Little changes from single thread into multiple, for example here [1]
>> > will
>> > > highlight a problem, or i can just publish my PR.
>> > >
>> > > [1]
>> > >
>> >
>>  https://github.com/apache/ignite/blob/master/modules/core/src/test/java/org/apache/ignite/internal/IgniteExplicitImplicitDeploymentSelfTest.java#L221
>> > >
>> > > >
>> > > >>
>> > > >>>Hello!
>> > > >>>
>> > > >>>Do you have some kind of reproducer which demonstrates the issue?
>> > > >>>
>> > > >>>Regards,
>> > > >>>--
>> > > >>>Ilya Kasnacheev
>> > > >>>
>> > > >>>
>> > > >>>чт, 28 янв. 2021 г. в 10:32, Zhenya Stanilovsky <
>> > >  arzamas123@mail.ru.invalid
>> > > >>>>:
>> > > >>>
>> > > >>>>
>> > > >>>> Hello Igniters !
>> > > >>>> In the process of Ignite usage i found that some part of Compute
>> > > >>>> functionality are thread unsafe and seems was designed with such
>> > > >>>> limitations initially.
>> > > >>>> Example : one (client, but it doesn`t matter at all) instance is
>> > > >>>> shared between numerous of fabric, all of them calls something
>> like
>> > :
>> > > >>>> IgniteCompute#execute(ComputeTask<T,R>, T)
>> > > >>>> or
>> > > >>>> IgniteCompute#execute(java.lang.Class<? extends ComputeTask<T,R>>,
>> > T)
>> > > >>>> and appropriate «async» methods — what kind of instance will be
>> > > called is
>> > > >>>> nondeterministic for now and as a confirmation of my words — i
>> found
>> > > no
>> > > >>>> tests covered multi thread usage of Computing i also found nothing
>> > on
>> > > >>>> documentation page [1].
>> > > >>>> We have all necessary info for correct processing of such cases:
>> > > >>>> from initiator (ignite.compute(...) starter) side we have Class or
>> > it
>> > > >>>> instance and appropriate class loader which will be wired by class
>> > > loader
>> > > >>>> id from execution side.
>> > > >>>> I create a fix and seems all work perfectly well besides one
>> place,
>> > > this
>> > > >>>> functionality :
>> > > >>>> /**
>> > > >>>> * Executes given task within the cluster group. For step-by-step
>> > > >>>> explanation of task execution process
>> > > >>>> * refer to {@link ComputeTask} documentation.
>> > > >>>> * <p>
>> > > >>>> * If task for given name has not been deployed yet, then {@code
>> > > taskName}
>> > > >>>> will be
>> > > >>>> * used as task class name to auto-deploy the task (see {@link
>> > > >>>> #localDeployTask(Class, ClassLoader)} method).
>> > > >>>> */
>> > > >>>> public <T, R> R execute(String taskName, T arg) throws
>> > > IgniteException;
>> > > >>>> and attendant
>> > > >>>> /**
>> > > >>>> * Finds class loader for the given class.
>> > > >>>> *
>> > > >>>> * @param rsrcName Class name or class alias to find class loader
>> > for.
>> > > >>>> * @return Deployed class loader, or {@code null} if not deployed.
>> > > >>>> */
>> > > >>>> public DeploymentResource findResource(String rsrcName);
>> > > >>>> is thread unsafe by default, no guarantee that concurrent call of
>> > > >>>> localDeployTask and execute will bring expected result.
>> > > >>>> My proposal is to deprecate (or probably annotate [2], as a
>> minimal
>> > > >>>> — additionally document it) this methods and to append additional
>> :
>> > > >>>> public DeploymentResource findResource(String rsrcName,
>> ClassLoader
>> > > >>>> clsLdr);
>> > > >>>> Only one problem i can observe here, if someone creates new class
>> > > loaders
>> > > >>>> and appropriate class instances in loop (i don`t know the purpose)
>> > and
>> > > >>>> doesn`t undeploy them then he will get possibly OOM here.
>> > > >>>>
>> > > >>>> Such approach will give a possibility to use compute in concurrent
>> > > >>>> scenario. If there is no objections here i will mark this methods
>> > and
>> > > >>>> publish my PR, of course with additional tests.
>> > > >>>>
>> > > >>>> What do you think ?
>> > > >>>>
>> > > >>>>
>> > > >>>> [1]
>> > > >>>>
>> > >
>>  https://ignite.apache.org/docs/latest/code-deployment/peer-class-loading
>> > > >>>> [2]
>> > > >>>>
>> > >
>>  https://jcip.net/annotations/doc/net/jcip/annotations/NotThreadSafe.html
>> > > >>>>
>> > > >>>>
>> > > >>
>> > > >>
>> > > >>
>> > > >>
>> >
>>
>>
>> --
>> Vladislav Pyatkov
>>