Re: IGNITE-8238 - Operation can fails with unexpected RuntimeException when node is stopping

[dsetrakyan]

As suggested before, please do not put blank ticket numbers into subjects
because no one understands ticket numbers. Please add titles or some other
context to the subject. This will improve the level of engagement from the
community.

I have changed the subject of this thread, let's continue the discussion
here.

D.

On Tue, May 29, 2018 at 9:10 AM, Александр Меньшиков <[hidden email]>
wrote:

> Hi, Andrew.
>
> You suggested continuing the discussion on dev-list about IGNITE-8238 [1]
> I have reworked my PR [2]. Have I moved in the right direction?
>
> I see 57 usages of `checkpointReadLock` in the core module (without tests).
> And it still unclear for me should I catch all exceptions from
> `checkpointReadLock` or not?
> Some places look okay like usage inside overrides of `GridWorker#body`.
> But for example `GridCacheAdapter#getAllAsync0` makes me worry.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-8238
> [2] https://github.com/apache/ignite/pull/3993/files
>

[Andrew Mashenkov]

Hi,

Yes, now you are on the right way.

It is ok if checkpointReadLock() throws runtime exception and in your PR
reason become obvious.
Now, it is easy to handle exceptions thrown from checkpointReadLock()
method on node stop if needed.

TtlManager looks correctly ignore NodeStopping exception, but I'd avoid
logging of warning message with stacktrace here.
I've left comments in ticket.

Normally, this change shouldn't affect any other behavior and you shouldn't
be bother to catch this exception on every checkpointReadLock call.
Those places where checkpointReadLock is called from, now fixed and will
correctly handle exception with cause (NodeStoppingExcpetion) if they tried
to do this before, of cause.

checkpointReadLock() could throw IgniteException before your changes. So
changing type RuntimeException -> IgniteException shouldn't have any affect
(otherwise it is a bug of incorrect exception handling).

Please run TC test and check if there is no new failures.



I see nothing in GridCacheAdapter#getAllAsync0 that may be affected by your
changes.


On Tue, May 29, 2018 at 8:05 PM, Dmitriy Setrakyan <[hidden email]>
wrote:

> As suggested before, please do not put blank ticket numbers into subjects
> because no one understands ticket numbers. Please add titles or some other
> context to the subject. This will improve the level of engagement from the
> community.
>
> I have changed the subject of this thread, let's continue the discussion
> here.
>
> D.
>
> On Tue, May 29, 2018 at 9:10 AM, Александр Меньшиков <[hidden email]
> > wrote:
>
>> Hi, Andrew.
>>
>> You suggested continuing the discussion on dev-list about IGNITE-8238 [1]
>> I have reworked my PR [2]. Have I moved in the right direction?
>>
>> I see 57 usages of `checkpointReadLock` in the core module (without
>> tests).
>> And it still unclear for me should I catch all exceptions from
>> `checkpointReadLock` or not?
>> Some places look okay like usage inside overrides of `GridWorker#body`.
>> But for example `GridCacheAdapter#getAllAsync0` makes me worry.
>>
>> [1] https://issues.apache.org/jira/browse/IGNITE-8238
>> [2] https://github.com/apache/ignite/pull/3993/files
>>
>
>

--
Best regards,
Andrey V. Mashenkov

[Александр Меньшиков]

Okay, I have changed the logging level to debug, updated master and rerun
TC tests [1].

[1]
https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_RunAll&branch_IgniteTests24Java8=pull%2F3993%2Fhead&tab=buildTypeStatusDiv

2018-05-30 16:13 GMT+03:00 Andrey Mashenkov <[hidden email]>:

> Hi,
>
> Yes, now you are on the right way.
>
> It is ok if checkpointReadLock() throws runtime exception and in your PR
> reason become obvious.
> Now, it is easy to handle exceptions thrown from checkpointReadLock()
> method on node stop if needed.
>
> TtlManager looks correctly ignore NodeStopping exception, but I'd avoid
> logging of warning message with stacktrace here.
> I've left comments in ticket.
>
> Normally, this change shouldn't affect any other behavior and you
> shouldn't be bother to catch this exception on every checkpointReadLock
> call.
> Those places where checkpointReadLock is called from, now fixed and will
> correctly handle exception with cause (NodeStoppingExcpetion) if they tried
> to do this before, of cause.
>
> checkpointReadLock() could throw IgniteException before your changes. So
> changing type RuntimeException -> IgniteException shouldn't have any
> affect (otherwise it is a bug of incorrect exception handling).
>
> Please run TC test and check if there is no new failures.
>
>
>
> I see nothing in GridCacheAdapter#getAllAsync0 that may be affected by
> your changes.
>
>
> On Tue, May 29, 2018 at 8:05 PM, Dmitriy Setrakyan <[hidden email]>
> wrote:
>
>> As suggested before, please do not put blank ticket numbers into subjects
>> because no one understands ticket numbers. Please add titles or some other
>> context to the subject. This will improve the level of engagement from the
>> community.
>>
>> I have changed the subject of this thread, let's continue the discussion
>> here.
>>
>> D.
>>
>> On Tue, May 29, 2018 at 9:10 AM, Александр Меньшиков <
>> [hidden email]> wrote:
>>
>>> Hi, Andrew.
>>>
>>> You suggested continuing the discussion on dev-list about IGNITE-8238 [1]
>>> I have reworked my PR [2]. Have I moved in the right direction?
>>>
>>> I see 57 usages of `checkpointReadLock` in the core module (without
>>> tests).
>>> And it still unclear for me should I catch all exceptions from
>>> `checkpointReadLock` or not?
>>> Some places look okay like usage inside overrides of `GridWorker#body`.
>>> But for example `GridCacheAdapter#getAllAsync0` makes me worry.
>>>
>>> [1] https://issues.apache.org/jira/browse/IGNITE-8238
>>> [2] https://github.com/apache/ignite/pull/3993/files
>>>
>>
>>
>
>
> --
> Best regards,
> Andrey V. Mashenkov
>