Service proxy API changes

[dkarachentsev]

Hi Igniters!

I'd like to modify our public API and add IgniteServices.serviceProxy()
method with timeout argument as part of the task
https://issues.apache.org/jira/browse/IGNITE-3862

In short, without timeout, in case of serialization error, or so, service
acquirement may hang and infinitely log errors.

Do you have any concerns about this change?

Thanks!
Dmitry.

[Vladimir Ozerov]

Dmitriy,

Shouldn't we report serialization problem properly instead? We already had
a problem when node hanged during job execution in case it was impossible
to deserialize the job on receiver side. It was resolved properly - we
caught exception on receiver side and reported it back sender side.

I believe we must do the same for services. Otherwise we may end up in
messy API which doesn't resolve original problem.

Vladimir.

On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
[hidden email]> wrote:

> Hi Igniters!
>
> I'd like to modify our public API and add IgniteServices.serviceProxy()
> method with timeout argument as part of the task
> https://issues.apache.org/jira/browse/IGNITE-3862
>
> In short, without timeout, in case of serialization error, or so, service
> acquirement may hang and infinitely log errors.
>
> Do you have any concerns about this change?
>
> Thanks!
> Dmitry.
>

[Vladimir Ozerov]

Also we implemented the same thing for platforms some time ago. In short,
job result processing was implemented as follows (pseudocode):

// Execute.
Object res;

try {
    res = job.run();
}
catch (Exception e) {
    res = e
}

// Serialize result.
try {
    SERIALIZE(res);
}
catch (Exception e) {
    try{
        // Serialize serialization error.
        SERIALIZE(new IgniteException("Failed to serialize result.", e));
    }
    catch (Exception e) {
        // Cannot serialize serialization error, so pass only string to
exception.
        SERIALIZE(new IgniteException("Failed to serialize result: " +
e.getMessage());
    }
}

On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <[hidden email]>
wrote:

> Dmitriy,
>
> Shouldn't we report serialization problem properly instead? We already had
> a problem when node hanged during job execution in case it was impossible
> to deserialize the job on receiver side. It was resolved properly - we
> caught exception on receiver side and reported it back sender side.
>
> I believe we must do the same for services. Otherwise we may end up in
> messy API which doesn't resolve original problem.
>
> Vladimir.
>
> On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
> [hidden email]> wrote:
>
>> Hi Igniters!
>>
>> I'd like to modify our public API and add IgniteServices.serviceProxy()
>> method with timeout argument as part of the task
>> https://issues.apache.org/jira/browse/IGNITE-3862
>>
>> In short, without timeout, in case of serialization error, or so, service
>> acquirement may hang and infinitely log errors.
>>
>> Do you have any concerns about this change?
>>
>> Thanks!
>> Dmitry.
>>
>
>

[dkarachentsev]

Vladimir, thanks for your reply!

What you suggest definitely makes sense and it looks more reasonable
solution.
But there remains other thing. The second issue, that this solution solves,
is prevent log pollution with GridServiceNotFoundException. The reason why
it happens is because GridServiceProxy#invokeMethod() designed to catch it
and ClusterTopologyCheckedException and retry over and over again unless
service is become available, but on the same time there are tons of stack
traces printed on remote node.

If we want to avoid changing API, probably we need to add option to mute
that exceptions in GridJobWorker.

What do you think?

On Tue, Nov 15, 2016 at 5:10 PM, Vladimir Ozerov <[hidden email]>
wrote:

> Also we implemented the same thing for platforms some time ago. In short,
> job result processing was implemented as follows (pseudocode):
>
> // Execute.
> Object res;
>
> try {
>     res = job.run();
> }
> catch (Exception e) {
>     res = e
> }
>
> // Serialize result.
> try {
>     SERIALIZE(res);
> }
> catch (Exception e) {
>     try{
>         // Serialize serialization error.
>         SERIALIZE(new IgniteException("Failed to serialize result.", e));
>     }
>     catch (Exception e) {
>         // Cannot serialize serialization error, so pass only string to
> exception.
>         SERIALIZE(new IgniteException("Failed to serialize result: " +
> e.getMessage());
>     }
> }
>
> On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > Dmitriy,
> >
> > Shouldn't we report serialization problem properly instead? We already
> had
> > a problem when node hanged during job execution in case it was impossible
> > to deserialize the job on receiver side. It was resolved properly - we
> > caught exception on receiver side and reported it back sender side.
> >
> > I believe we must do the same for services. Otherwise we may end up in
> > messy API which doesn't resolve original problem.
> >
> > Vladimir.
> >
> > On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
> > [hidden email]> wrote:
> >
> >> Hi Igniters!
> >>
> >> I'd like to modify our public API and add IgniteServices.serviceProxy()
> >> method with timeout argument as part of the task
> >> https://issues.apache.org/jira/browse/IGNITE-3862
> >>
> >> In short, without timeout, in case of serialization error, or so,
> service
> >> acquirement may hang and infinitely log errors.
> >>
> >> Do you have any concerns about this change?
> >>
> >> Thanks!
> >> Dmitry.
> >>
> >
> >
>

[Vladimir Ozerov]

To avoid log pollution we usually use LT class (alias for GridLogThrottle).
Please check if it can help you.

On Tue, Nov 15, 2016 at 5:48 PM, Dmitriy Karachentsev <
[hidden email]> wrote:

> Vladimir, thanks for your reply!
>
> What you suggest definitely makes sense and it looks more reasonable
> solution.
> But there remains other thing. The second issue, that this solution solves,
> is prevent log pollution with GridServiceNotFoundException. The reason why
> it happens is because GridServiceProxy#invokeMethod() designed to catch it
> and ClusterTopologyCheckedException and retry over and over again unless
> service is become available, but on the same time there are tons of stack
> traces printed on remote node.
>
> If we want to avoid changing API, probably we need to add option to mute
> that exceptions in GridJobWorker.
>
> What do you think?
>
> On Tue, Nov 15, 2016 at 5:10 PM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > Also we implemented the same thing for platforms some time ago. In short,
> > job result processing was implemented as follows (pseudocode):
> >
> > // Execute.
> > Object res;
> >
> > try {
> >     res = job.run();
> > }
> > catch (Exception e) {
> >     res = e
> > }
> >
> > // Serialize result.
> > try {
> >     SERIALIZE(res);
> > }
> > catch (Exception e) {
> >     try{
> >         // Serialize serialization error.
> >         SERIALIZE(new IgniteException("Failed to serialize result.", e));
> >     }
> >     catch (Exception e) {
> >         // Cannot serialize serialization error, so pass only string to
> > exception.
> >         SERIALIZE(new IgniteException("Failed to serialize result: " +
> > e.getMessage());
> >     }
> > }
> >
> > On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <[hidden email]>
> > wrote:
> >
> > > Dmitriy,
> > >
> > > Shouldn't we report serialization problem properly instead? We already
> > had
> > > a problem when node hanged during job execution in case it was
> impossible
> > > to deserialize the job on receiver side. It was resolved properly - we
> > > caught exception on receiver side and reported it back sender side.
> > >
> > > I believe we must do the same for services. Otherwise we may end up in
> > > messy API which doesn't resolve original problem.
> > >
> > > Vladimir.
> > >
> > > On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
> > > [hidden email]> wrote:
> > >
> > >> Hi Igniters!
> > >>
> > >> I'd like to modify our public API and add
> IgniteServices.serviceProxy()
> > >> method with timeout argument as part of the task
> > >> https://issues.apache.org/jira/browse/IGNITE-3862
> > >>
> > >> In short, without timeout, in case of serialization error, or so,
> > service
> > >> acquirement may hang and infinitely log errors.
> > >>
> > >> Do you have any concerns about this change?
> > >>
> > >> Thanks!
> > >> Dmitry.
> > >>
> > >
> > >
> >
>

[dkarachentsev]

Perfect, thanks!

On Tue, Nov 15, 2016 at 5:56 PM, Vladimir Ozerov <[hidden email]>
wrote:

> To avoid log pollution we usually use LT class (alias for GridLogThrottle).
> Please check if it can help you.
>
> On Tue, Nov 15, 2016 at 5:48 PM, Dmitriy Karachentsev <
> [hidden email]> wrote:
>
> > Vladimir, thanks for your reply!
> >
> > What you suggest definitely makes sense and it looks more reasonable
> > solution.
> > But there remains other thing. The second issue, that this solution
> solves,
> > is prevent log pollution with GridServiceNotFoundException. The reason
> why
> > it happens is because GridServiceProxy#invokeMethod() designed to catch
> it
> > and ClusterTopologyCheckedException and retry over and over again unless
> > service is become available, but on the same time there are tons of stack
> > traces printed on remote node.
> >
> > If we want to avoid changing API, probably we need to add option to mute
> > that exceptions in GridJobWorker.
> >
> > What do you think?
> >
> > On Tue, Nov 15, 2016 at 5:10 PM, Vladimir Ozerov <[hidden email]>
> > wrote:
> >
> > > Also we implemented the same thing for platforms some time ago. In
> short,
> > > job result processing was implemented as follows (pseudocode):
> > >
> > > // Execute.
> > > Object res;
> > >
> > > try {
> > >     res = job.run();
> > > }
> > > catch (Exception e) {
> > >     res = e
> > > }
> > >
> > > // Serialize result.
> > > try {
> > >     SERIALIZE(res);
> > > }
> > > catch (Exception e) {
> > >     try{
> > >         // Serialize serialization error.
> > >         SERIALIZE(new IgniteException("Failed to serialize result.",
> e));
> > >     }
> > >     catch (Exception e) {
> > >         // Cannot serialize serialization error, so pass only string to
> > > exception.
> > >         SERIALIZE(new IgniteException("Failed to serialize result: " +
> > > e.getMessage());
> > >     }
> > > }
> > >
> > > On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <[hidden email]
> >
> > > wrote:
> > >
> > > > Dmitriy,
> > > >
> > > > Shouldn't we report serialization problem properly instead? We
> already
> > > had
> > > > a problem when node hanged during job execution in case it was
> > impossible
> > > > to deserialize the job on receiver side. It was resolved properly -
> we
> > > > caught exception on receiver side and reported it back sender side.
> > > >
> > > > I believe we must do the same for services. Otherwise we may end up
> in
> > > > messy API which doesn't resolve original problem.
> > > >
> > > > Vladimir.
> > > >
> > > > On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
> > > > [hidden email]> wrote:
> > > >
> > > >> Hi Igniters!
> > > >>
> > > >> I'd like to modify our public API and add
> > IgniteServices.serviceProxy()
> > > >> method with timeout argument as part of the task
> > > >> https://issues.apache.org/jira/browse/IGNITE-3862
> > > >>
> > > >> In short, without timeout, in case of serialization error, or so,
> > > service
> > > >> acquirement may hang and infinitely log errors.
> > > >>
> > > >> Do you have any concerns about this change?
> > > >>
> > > >> Thanks!
> > > >> Dmitry.
> > > >>
> > > >
> > > >
> > >
> >
>

[Valentin Kulichenko]

I would still add a timeout there. In my view, it makes sense to have such
option. Currently user thread can block indefinitely or loop in while(true)
forever.

-Val

On Tue, Nov 15, 2016 at 7:10 AM, Dmitriy Karachentsev <
[hidden email]> wrote:

> Perfect, thanks!
>
> On Tue, Nov 15, 2016 at 5:56 PM, Vladimir Ozerov <[hidden email]>
> wrote:
>
> > To avoid log pollution we usually use LT class (alias for
> GridLogThrottle).
> > Please check if it can help you.
> >
> > On Tue, Nov 15, 2016 at 5:48 PM, Dmitriy Karachentsev <
> > [hidden email]> wrote:
> >
> > > Vladimir, thanks for your reply!
> > >
> > > What you suggest definitely makes sense and it looks more reasonable
> > > solution.
> > > But there remains other thing. The second issue, that this solution
> > solves,
> > > is prevent log pollution with GridServiceNotFoundException. The reason
> > why
> > > it happens is because GridServiceProxy#invokeMethod() designed to
> catch
> > it
> > > and ClusterTopologyCheckedException and retry over and over again
> unless
> > > service is become available, but on the same time there are tons of
> stack
> > > traces printed on remote node.
> > >
> > > If we want to avoid changing API, probably we need to add option to
> mute
> > > that exceptions in GridJobWorker.
> > >
> > > What do you think?
> > >
> > > On Tue, Nov 15, 2016 at 5:10 PM, Vladimir Ozerov <[hidden email]
> >
> > > wrote:
> > >
> > > > Also we implemented the same thing for platforms some time ago. In
> > short,
> > > > job result processing was implemented as follows (pseudocode):
> > > >
> > > > // Execute.
> > > > Object res;
> > > >
> > > > try {
> > > >     res = job.run();
> > > > }
> > > > catch (Exception e) {
> > > >     res = e
> > > > }
> > > >
> > > > // Serialize result.
> > > > try {
> > > >     SERIALIZE(res);
> > > > }
> > > > catch (Exception e) {
> > > >     try{
> > > >         // Serialize serialization error.
> > > >         SERIALIZE(new IgniteException("Failed to serialize result.",
> > e));
> > > >     }
> > > >     catch (Exception e) {
> > > >         // Cannot serialize serialization error, so pass only string
> to
> > > > exception.
> > > >         SERIALIZE(new IgniteException("Failed to serialize result: "
> +
> > > > e.getMessage());
> > > >     }
> > > > }
> > > >
> > > > On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <
> [hidden email]
> > >
> > > > wrote:
> > > >
> > > > > Dmitriy,
> > > > >
> > > > > Shouldn't we report serialization problem properly instead? We
> > already
> > > > had
> > > > > a problem when node hanged during job execution in case it was
> > > impossible
> > > > > to deserialize the job on receiver side. It was resolved properly -
> > we
> > > > > caught exception on receiver side and reported it back sender side.
> > > > >
> > > > > I believe we must do the same for services. Otherwise we may end up
> > in
> > > > > messy API which doesn't resolve original problem.
> > > > >
> > > > > Vladimir.
> > > > >
> > > > > On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
> > > > > [hidden email]> wrote:
> > > > >
> > > > >> Hi Igniters!
> > > > >>
> > > > >> I'd like to modify our public API and add
> > > IgniteServices.serviceProxy()
> > > > >> method with timeout argument as part of the task
> > > > >> https://issues.apache.org/jira/browse/IGNITE-3862
> > > > >>
> > > > >> In short, without timeout, in case of serialization error, or so,
> > > > service
> > > > >> acquirement may hang and infinitely log errors.
> > > > >>
> > > > >> Do you have any concerns about this change?
> > > > >>
> > > > >> Thanks!
> > > > >> Dmitry.
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
>

[dkarachentsev]

Valentin, as I understand, this situation is abnormal, and if user
thread hangs on getting service proxy, it's worth to check logs for
failure instead of making workaround.

On 15.11.2016 19:47, Valentin Kulichenko wrote:

> I would still add a timeout there. In my view, it makes sense to have such
> option. Currently user thread can block indefinitely or loop in while(true)
> forever.
>
> -Val
>
> On Tue, Nov 15, 2016 at 7:10 AM, Dmitriy Karachentsev <
> [hidden email]> wrote:
>
>> Perfect, thanks!
>>
>> On Tue, Nov 15, 2016 at 5:56 PM, Vladimir Ozerov <[hidden email]>
>> wrote:
>>
>>> To avoid log pollution we usually use LT class (alias for
>> GridLogThrottle).
>>> Please check if it can help you.
>>>
>>> On Tue, Nov 15, 2016 at 5:48 PM, Dmitriy Karachentsev <
>>> [hidden email]> wrote:
>>>
>>>> Vladimir, thanks for your reply!
>>>>
>>>> What you suggest definitely makes sense and it looks more reasonable
>>>> solution.
>>>> But there remains other thing. The second issue, that this solution
>>> solves,
>>>> is prevent log pollution with GridServiceNotFoundException. The reason
>>> why
>>>> it happens is because GridServiceProxy#invokeMethod() designed to
>> catch
>>> it
>>>> and ClusterTopologyCheckedException and retry over and over again
>> unless
>>>> service is become available, but on the same time there are tons of
>> stack
>>>> traces printed on remote node.
>>>>
>>>> If we want to avoid changing API, probably we need to add option to
>> mute
>>>> that exceptions in GridJobWorker.
>>>>
>>>> What do you think?
>>>>
>>>> On Tue, Nov 15, 2016 at 5:10 PM, Vladimir Ozerov <[hidden email]
>>>> wrote:
>>>>
>>>>> Also we implemented the same thing for platforms some time ago. In
>>> short,
>>>>> job result processing was implemented as follows (pseudocode):
>>>>>
>>>>> // Execute.
>>>>> Object res;
>>>>>
>>>>> try {
>>>>>      res = job.run();
>>>>> }
>>>>> catch (Exception e) {
>>>>>      res = e
>>>>> }
>>>>>
>>>>> // Serialize result.
>>>>> try {
>>>>>      SERIALIZE(res);
>>>>> }
>>>>> catch (Exception e) {
>>>>>      try{
>>>>>          // Serialize serialization error.
>>>>>          SERIALIZE(new IgniteException("Failed to serialize result.",
>>> e));
>>>>>      }
>>>>>      catch (Exception e) {
>>>>>          // Cannot serialize serialization error, so pass only string
>> to
>>>>> exception.
>>>>>          SERIALIZE(new IgniteException("Failed to serialize result: "
>> +
>>>>> e.getMessage());
>>>>>      }
>>>>> }
>>>>>
>>>>> On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <
>> [hidden email]
>>>>> wrote:
>>>>>
>>>>>> Dmitriy,
>>>>>>
>>>>>> Shouldn't we report serialization problem properly instead? We
>>> already
>>>>> had
>>>>>> a problem when node hanged during job execution in case it was
>>>> impossible
>>>>>> to deserialize the job on receiver side. It was resolved properly -
>>> we
>>>>>> caught exception on receiver side and reported it back sender side.
>>>>>>
>>>>>> I believe we must do the same for services. Otherwise we may end up
>>> in
>>>>>> messy API which doesn't resolve original problem.
>>>>>>
>>>>>> Vladimir.
>>>>>>
>>>>>> On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
>>>>>> [hidden email]> wrote:
>>>>>>
>>>>>>> Hi Igniters!
>>>>>>>
>>>>>>> I'd like to modify our public API and add
>>>> IgniteServices.serviceProxy()
>>>>>>> method with timeout argument as part of the task
>>>>>>> https://issues.apache.org/jira/browse/IGNITE-3862
>>>>>>>
>>>>>>> In short, without timeout, in case of serialization error, or so,
>>>>> service
>>>>>>> acquirement may hang and infinitely log errors.
>>>>>>>
>>>>>>> Do you have any concerns about this change?
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Dmitry.
>>>>>>>
>>>>>>

[Valentin Kulichenko]

Dmitry,

I absolutely agree with this and I'm not saying that timeout solves any of
these problems. However, it provides a bit of safety for such cases (client
continues working instead of hanging forever) and also allows not too wait
for the service too long which can be also useful sometimes (e.g. service
is redeployed and initialization takes a lot of time for some reason). It
makes perfect sense to have it and should not be difficult to add. Am I
missing something?

-Val

On Tue, Nov 15, 2016 at 1:12 PM, Dmitry Karachentsev <
[hidden email]> wrote:

> Valentin, as I understand, this situation is abnormal, and if user thread
> hangs on getting service proxy, it's worth to check logs for failure
> instead of making workaround.
>
>
> On 15.11.2016 19:47, Valentin Kulichenko wrote:
>
>> I would still add a timeout there. In my view, it makes sense to have such
>> option. Currently user thread can block indefinitely or loop in
>> while(true)
>> forever.
>>
>> -Val
>>
>> On Tue, Nov 15, 2016 at 7:10 AM, Dmitriy Karachentsev <
>> [hidden email]> wrote:
>>
>> Perfect, thanks!
>>>
>>> On Tue, Nov 15, 2016 at 5:56 PM, Vladimir Ozerov <[hidden email]>
>>> wrote:
>>>
>>> To avoid log pollution we usually use LT class (alias for
>>>>
>>> GridLogThrottle).
>>>
>>>> Please check if it can help you.
>>>>
>>>> On Tue, Nov 15, 2016 at 5:48 PM, Dmitriy Karachentsev <
>>>> [hidden email]> wrote:
>>>>
>>>> Vladimir, thanks for your reply!
>>>>>
>>>>> What you suggest definitely makes sense and it looks more reasonable
>>>>> solution.
>>>>> But there remains other thing. The second issue, that this solution
>>>>>
>>>> solves,
>>>>
>>>>> is prevent log pollution with GridServiceNotFoundException. The reason
>>>>>
>>>> why
>>>>
>>>>> it happens is because GridServiceProxy#invokeMethod() designed to
>>>>>
>>>> catch
>>>
>>>> it
>>>>
>>>>> and ClusterTopologyCheckedException and retry over and over again
>>>>>
>>>> unless
>>>
>>>> service is become available, but on the same time there are tons of
>>>>>
>>>> stack
>>>
>>>> traces printed on remote node.
>>>>>
>>>>> If we want to avoid changing API, probably we need to add option to
>>>>>
>>>> mute
>>>
>>>> that exceptions in GridJobWorker.
>>>>>
>>>>> What do you think?
>>>>>
>>>>> On Tue, Nov 15, 2016 at 5:10 PM, Vladimir Ozerov <[hidden email]
>>>>> wrote:
>>>>>
>>>>> Also we implemented the same thing for platforms some time ago. In
>>>>>>
>>>>> short,
>>>>
>>>>> job result processing was implemented as follows (pseudocode):
>>>>>>
>>>>>> // Execute.
>>>>>> Object res;
>>>>>>
>>>>>> try {
>>>>>>      res = job.run();
>>>>>> }
>>>>>> catch (Exception e) {
>>>>>>      res = e
>>>>>> }
>>>>>>
>>>>>> // Serialize result.
>>>>>> try {
>>>>>>      SERIALIZE(res);
>>>>>> }
>>>>>> catch (Exception e) {
>>>>>>      try{
>>>>>>          // Serialize serialization error.
>>>>>>          SERIALIZE(new IgniteException("Failed to serialize result.",
>>>>>>
>>>>> e));
>>>>
>>>>>      }
>>>>>>      catch (Exception e) {
>>>>>>          // Cannot serialize serialization error, so pass only string
>>>>>>
>>>>> to
>>>
>>>> exception.
>>>>>>          SERIALIZE(new IgniteException("Failed to serialize result: "
>>>>>>
>>>>> +
>>>
>>>> e.getMessage());
>>>>>>      }
>>>>>> }
>>>>>>
>>>>>> On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <
>>>>>>
>>>>> [hidden email]
>>>
>>>> wrote:
>>>>>>
>>>>>> Dmitriy,
>>>>>>>
>>>>>>> Shouldn't we report serialization problem properly instead? We
>>>>>>>
>>>>>> already
>>>>
>>>>> had
>>>>>>
>>>>>>> a problem when node hanged during job execution in case it was
>>>>>>>
>>>>>> impossible
>>>>>
>>>>>> to deserialize the job on receiver side. It was resolved properly -
>>>>>>>
>>>>>> we
>>>>
>>>>> caught exception on receiver side and reported it back sender side.
>>>>>>>
>>>>>>> I believe we must do the same for services. Otherwise we may end up
>>>>>>>
>>>>>> in
>>>>
>>>>> messy API which doesn't resolve original problem.
>>>>>>>
>>>>>>> Vladimir.
>>>>>>>
>>>>>>> On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
>>>>>>> [hidden email]> wrote:
>>>>>>>
>>>>>>> Hi Igniters!
>>>>>>>>
>>>>>>>> I'd like to modify our public API and add
>>>>>>>>
>>>>>>> IgniteServices.serviceProxy()
>>>>>
>>>>>> method with timeout argument as part of the task
>>>>>>>> https://issues.apache.org/jira/browse/IGNITE-3862
>>>>>>>>
>>>>>>>> In short, without timeout, in case of serialization error, or so,
>>>>>>>>
>>>>>>> service
>>>>>>
>>>>>>> acquirement may hang and infinitely log errors.
>>>>>>>>
>>>>>>>> Do you have any concerns about this change?
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>> Dmitry.
>>>>>>>>
>>>>>>>>
>>>>>>>
>

[dkarachentsev]

Valentin,

I understand your point and it makes sense to me. To summarize discussion
I'll do the following. Leave timeout, use LT for spammed stack traces and
file separate ticket for serialization issue in calculations as suggested
Semyon.

On Wed, Nov 16, 2016 at 1:11 AM, Valentin Kulichenko <
[hidden email]> wrote:

> Dmitry,
>
> I absolutely agree with this and I'm not saying that timeout solves any of
> these problems. However, it provides a bit of safety for such cases (client
> continues working instead of hanging forever) and also allows not too wait
> for the service too long which can be also useful sometimes (e.g. service
> is redeployed and initialization takes a lot of time for some reason). It
> makes perfect sense to have it and should not be difficult to add. Am I
> missing something?
>
> -Val
>
> On Tue, Nov 15, 2016 at 1:12 PM, Dmitry Karachentsev <
> [hidden email]> wrote:
>
> > Valentin, as I understand, this situation is abnormal, and if user thread
> > hangs on getting service proxy, it's worth to check logs for failure
> > instead of making workaround.
> >
> >
> > On 15.11.2016 19:47, Valentin Kulichenko wrote:
> >
> >> I would still add a timeout there. In my view, it makes sense to have
> such
> >> option. Currently user thread can block indefinitely or loop in
> >> while(true)
> >> forever.
> >>
> >> -Val
> >>
> >> On Tue, Nov 15, 2016 at 7:10 AM, Dmitriy Karachentsev <
> >> [hidden email]> wrote:
> >>
> >> Perfect, thanks!
> >>>
> >>> On Tue, Nov 15, 2016 at 5:56 PM, Vladimir Ozerov <[hidden email]
> >
> >>> wrote:
> >>>
> >>> To avoid log pollution we usually use LT class (alias for
> >>>>
> >>> GridLogThrottle).
> >>>
> >>>> Please check if it can help you.
> >>>>
> >>>> On Tue, Nov 15, 2016 at 5:48 PM, Dmitriy Karachentsev <
> >>>> [hidden email]> wrote:
> >>>>
> >>>> Vladimir, thanks for your reply!
> >>>>>
> >>>>> What you suggest definitely makes sense and it looks more reasonable
> >>>>> solution.
> >>>>> But there remains other thing. The second issue, that this solution
> >>>>>
> >>>> solves,
> >>>>
> >>>>> is prevent log pollution with GridServiceNotFoundException. The
> reason
> >>>>>
> >>>> why
> >>>>
> >>>>> it happens is because GridServiceProxy#invokeMethod() designed to
> >>>>>
> >>>> catch
> >>>
> >>>> it
> >>>>
> >>>>> and ClusterTopologyCheckedException and retry over and over again
> >>>>>
> >>>> unless
> >>>
> >>>> service is become available, but on the same time there are tons of
> >>>>>
> >>>> stack
> >>>
> >>>> traces printed on remote node.
> >>>>>
> >>>>> If we want to avoid changing API, probably we need to add option to
> >>>>>
> >>>> mute
> >>>
> >>>> that exceptions in GridJobWorker.
> >>>>>
> >>>>> What do you think?
> >>>>>
> >>>>> On Tue, Nov 15, 2016 at 5:10 PM, Vladimir Ozerov <
> [hidden email]
> >>>>> wrote:
> >>>>>
> >>>>> Also we implemented the same thing for platforms some time ago. In
> >>>>>>
> >>>>> short,
> >>>>
> >>>>> job result processing was implemented as follows (pseudocode):
> >>>>>>
> >>>>>> // Execute.
> >>>>>> Object res;
> >>>>>>
> >>>>>> try {
> >>>>>>      res = job.run();
> >>>>>> }
> >>>>>> catch (Exception e) {
> >>>>>>      res = e
> >>>>>> }
> >>>>>>
> >>>>>> // Serialize result.
> >>>>>> try {
> >>>>>>      SERIALIZE(res);
> >>>>>> }
> >>>>>> catch (Exception e) {
> >>>>>>      try{
> >>>>>>          // Serialize serialization error.
> >>>>>>          SERIALIZE(new IgniteException("Failed to serialize
> result.",
> >>>>>>
> >>>>> e));
> >>>>
> >>>>>      }
> >>>>>>      catch (Exception e) {
> >>>>>>          // Cannot serialize serialization error, so pass only
> string
> >>>>>>
> >>>>> to
> >>>
> >>>> exception.
> >>>>>>          SERIALIZE(new IgniteException("Failed to serialize result:
> "
> >>>>>>
> >>>>> +
> >>>
> >>>> e.getMessage());
> >>>>>>      }
> >>>>>> }
> >>>>>>
> >>>>>> On Tue, Nov 15, 2016 at 5:05 PM, Vladimir Ozerov <
> >>>>>>
> >>>>> [hidden email]
> >>>
> >>>> wrote:
> >>>>>>
> >>>>>> Dmitriy,
> >>>>>>>
> >>>>>>> Shouldn't we report serialization problem properly instead? We
> >>>>>>>
> >>>>>> already
> >>>>
> >>>>> had
> >>>>>>
> >>>>>>> a problem when node hanged during job execution in case it was
> >>>>>>>
> >>>>>> impossible
> >>>>>
> >>>>>> to deserialize the job on receiver side. It was resolved properly -
> >>>>>>>
> >>>>>> we
> >>>>
> >>>>> caught exception on receiver side and reported it back sender side.
> >>>>>>>
> >>>>>>> I believe we must do the same for services. Otherwise we may end up
> >>>>>>>
> >>>>>> in
> >>>>
> >>>>> messy API which doesn't resolve original problem.
> >>>>>>>
> >>>>>>> Vladimir.
> >>>>>>>
> >>>>>>> On Tue, Nov 15, 2016 at 4:38 PM, Dmitriy Karachentsev <
> >>>>>>> [hidden email]> wrote:
> >>>>>>>
> >>>>>>> Hi Igniters!
> >>>>>>>>
> >>>>>>>> I'd like to modify our public API and add
> >>>>>>>>
> >>>>>>> IgniteServices.serviceProxy()
> >>>>>
> >>>>>> method with timeout argument as part of the task
> >>>>>>>> https://issues.apache.org/jira/browse/IGNITE-3862
> >>>>>>>>
> >>>>>>>> In short, without timeout, in case of serialization error, or so,
> >>>>>>>>
> >>>>>>> service
> >>>>>>
> >>>>>>> acquirement may hang and infinitely log errors.
> >>>>>>>>
> >>>>>>>> Do you have any concerns about this change?
> >>>>>>>>
> >>>>>>>> Thanks!
> >>>>>>>> Dmitry.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>
> >
>