Switching to real FailureHandler in tests

[Andrey Kuznetsov]

Igniters,

While running tests I see a lot of ignored critical failures caused by
{{NoOpFailureHandler}} that we use by default. In some tests, of cource,
critical failures are the part of normal workflow, but in the majority of
tests they indicate bugs. By using {{NoOpFailureHandler}} we just hide
these bugs from ourselves.

What do you think about changing default handler to
{{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This could be
overridden in subclasses.

--
Best regards,
  Andrey Kuznetsov.

[Dmitriy Pavlov]

++1

чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <[hidden email]>:

> Igniters,
>
> While running tests I see a lot of ignored critical failures caused by
> {{NoOpFailureHandler}} that we use by default. In some tests, of cource,
> critical failures are the part of normal workflow, but in the majority of
> tests they indicate bugs. By using {{NoOpFailureHandler}} we just hide
> these bugs from ourselves.
>
> What do you think about changing default handler to
> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This could be
> overridden in subclasses.
>
> --
> Best regards,
>   Andrey Kuznetsov.
>

[Dmitriy Pavlov]

But the totally ideal situation would be finding a way to fail the test by
default, not only stopping a node.

Some time ago I've created https://issues.apache.org/jira/browse/IGNITE-8227 to
find out a way to do so.

чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <[hidden email]>:

> ++1
>
> чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <[hidden email]>:
>
>> Igniters,
>>
>> While running tests I see a lot of ignored critical failures caused by
>> {{NoOpFailureHandler}} that we use by default. In some tests, of cource,
>> critical failures are the part of normal workflow, but in the majority of
>> tests they indicate bugs. By using {{NoOpFailureHandler}} we just hide
>> these bugs from ourselves.
>>
>> What do you think about changing default handler to
>> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This could be
>> overridden in subclasses.
>>
>> --
>> Best regards,
>>   Andrey Kuznetsov.
>>
>

[Mmuzaf]

Andrey,

I like your idea.

After changing the default node failure handler to the new one we should
carefully review the whole new test failures. For instance, calling this
method in tests should not lead test to the node being stopped:

<strong>FOR TEST ONLY!!!</strong>
TcpDiscoverySpi#simulateNodeFailure

BTW, I would like to remove this method at all from production code.

On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <[hidden email]> wrote:

> But the totally ideal situation would be finding a way to fail the test by
> default, not only stopping a node.
>
> Some time ago I've created
> https://issues.apache.org/jira/browse/IGNITE-8227 to
> find out a way to do so.
>
> чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <[hidden email]>:
>
> > ++1
> >
> > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <[hidden email]>:
> >
> >> Igniters,
> >>
> >> While running tests I see a lot of ignored critical failures caused by
> >> {{NoOpFailureHandler}} that we use by default. In some tests, of cource,
> >> critical failures are the part of normal workflow, but in the majority
> of
> >> tests they indicate bugs. By using {{NoOpFailureHandler}} we just hide
> >> these bugs from ourselves.
> >>
> >> What do you think about changing default handler to
> >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This could be
> >> overridden in subclasses.
> >>
> >> --
> >> Best regards,
> >>   Andrey Kuznetsov.
> >>
> >
>

--
--
Maxim Muzafarov

[Andrey Kuznetsov]

I've created [1] to address this.

Dmitriy, I like your idea of creating special test-scope handler. But there
is no consensus about it, so I don't want to rely on that potential handler
right now. We can switch to it later, of course.

[1] https://issues.apache.org/jira/browse/IGNITE-9660

чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <[hidden email]>:

> Andrey,
>
> I like your idea.
>
> After changing the default node failure handler to the new one we should
> carefully review the whole new test failures. For instance, calling this
> method in tests should not lead test to the node being stopped:
>
> <strong>FOR TEST ONLY!!!</strong>
> TcpDiscoverySpi#simulateNodeFailure
>
> BTW, I would like to remove this method at all from production code.
>
> On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <[hidden email]> wrote:
>
> > But the totally ideal situation would be finding a way to fail the test
> by
> > default, not only stopping a node.
> >
> > Some time ago I've created
> > https://issues.apache.org/jira/browse/IGNITE-8227 to
> > find out a way to do so.
> >
> > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <[hidden email]>:
> >
> > > ++1
> > >
> > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <[hidden email]>:
> > >
> > >> Igniters,
> > >>
> > >> While running tests I see a lot of ignored critical failures caused by
> > >> {{NoOpFailureHandler}} that we use by default. In some tests, of
> cource,
> > >> critical failures are the part of normal workflow, but in the majority
> > of
> > >> tests they indicate bugs. By using {{NoOpFailureHandler}} we just hide
> > >> these bugs from ourselves.
> > >>
> > >> What do you think about changing default handler to
> > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This could
> be
> > >> overridden in subclasses.
> > >>
> > >> --
> > >> Best regards,
> > >>   Andrey Kuznetsov.
> > >>
> > >
> >
> --
> --
> Maxim Muzafarov
>

--
Best regards,
  Andrey Kuznetsov.

[Dmitriy Pavlov]

Why do you think there is no consensus?

I have no clue that by default failure handler should fail all test.

чт, 20 сент. 2018 г. в 21:10, Andrey Kuznetsov <[hidden email]>:

> I've created [1] to address this.
>
> Dmitriy, I like your idea of creating special test-scope handler. But there
> is no consensus about it, so I don't want to rely on that potential handler
> right now. We can switch to it later, of course.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-9660
>
> чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <[hidden email]>:
>
> > Andrey,
> >
> > I like your idea.
> >
> > After changing the default node failure handler to the new one we should
> > carefully review the whole new test failures. For instance, calling this
> > method in tests should not lead test to the node being stopped:
> >
> > <strong>FOR TEST ONLY!!!</strong>
> > TcpDiscoverySpi#simulateNodeFailure
> >
> > BTW, I would like to remove this method at all from production code.
> >
> > On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <[hidden email]>
> wrote:
> >
> > > But the totally ideal situation would be finding a way to fail the test
> > by
> > > default, not only stopping a node.
> > >
> > > Some time ago I've created
> > > https://issues.apache.org/jira/browse/IGNITE-8227 to
> > > find out a way to do so.
> > >
> > > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <[hidden email]>:
> > >
> > > > ++1
> > > >
> > > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <[hidden email]>:
> > > >
> > > >> Igniters,
> > > >>
> > > >> While running tests I see a lot of ignored critical failures caused
> by
> > > >> {{NoOpFailureHandler}} that we use by default. In some tests, of
> > cource,
> > > >> critical failures are the part of normal workflow, but in the
> majority
> > > of
> > > >> tests they indicate bugs. By using {{NoOpFailureHandler}} we just
> hide
> > > >> these bugs from ourselves.
> > > >>
> > > >> What do you think about changing default handler to
> > > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This could
> > be
> > > >> overridden in subclasses.
> > > >>
> > > >> --
> > > >> Best regards,
> > > >>   Andrey Kuznetsov.
> > > >>
> > > >
> > >
> > --
> > --
> > Maxim Muzafarov
> >
>
>
> --
> Best regards,
>   Andrey Kuznetsov.
>

[Dmitriy Pavlov]

Sorry, incomplete message.

Why do you think there is no consensus?

I have no clue what can be a reason for another approach.
By default failure handler should fail all test.

Failure handlers test will be always a minority of tests, so fail handler
call is something abnormal.

чт, 20 сент. 2018 г. в 21:15, Dmitriy Pavlov <[hidden email]>:

> Why do you think there is no consensus?
>
> I have no clue that by default failure handler should fail all test.
>
> чт, 20 сент. 2018 г. в 21:10, Andrey Kuznetsov <[hidden email]>:
>
>> I've created [1] to address this.
>>
>> Dmitriy, I like your idea of creating special test-scope handler. But
>> there
>> is no consensus about it, so I don't want to rely on that potential
>> handler
>> right now. We can switch to it later, of course.
>>
>> [1] https://issues.apache.org/jira/browse/IGNITE-9660
>>
>> чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <[hidden email]>:
>>
>> > Andrey,
>> >
>> > I like your idea.
>> >
>> > After changing the default node failure handler to the new one we should
>> > carefully review the whole new test failures. For instance, calling this
>> > method in tests should not lead test to the node being stopped:
>> >
>> > <strong>FOR TEST ONLY!!!</strong>
>> > TcpDiscoverySpi#simulateNodeFailure
>> >
>> > BTW, I would like to remove this method at all from production code.
>> >
>> > On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <[hidden email]>
>> wrote:
>> >
>> > > But the totally ideal situation would be finding a way to fail the
>> test
>> > by
>> > > default, not only stopping a node.
>> > >
>> > > Some time ago I've created
>> > > https://issues.apache.org/jira/browse/IGNITE-8227 to
>> > > find out a way to do so.
>> > >
>> > > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <[hidden email]>:
>> > >
>> > > > ++1
>> > > >
>> > > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <[hidden email]>:
>> > > >
>> > > >> Igniters,
>> > > >>
>> > > >> While running tests I see a lot of ignored critical failures
>> caused by
>> > > >> {{NoOpFailureHandler}} that we use by default. In some tests, of
>> > cource,
>> > > >> critical failures are the part of normal workflow, but in the
>> majority
>> > > of
>> > > >> tests they indicate bugs. By using {{NoOpFailureHandler}} we just
>> hide
>> > > >> these bugs from ourselves.
>> > > >>
>> > > >> What do you think about changing default handler to
>> > > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This
>> could
>> > be
>> > > >> overridden in subclasses.
>> > > >>
>> > > >> --
>> > > >> Best regards,
>> > > >>   Andrey Kuznetsov.
>> > > >>
>> > > >
>> > >
>> > --
>> > --
>> > Maxim Muzafarov
>> >
>>
>>
>> --
>> Best regards,
>>   Andrey Kuznetsov.
>>
>

[Andrey Kuznetsov]

I meant the first comment in [1]. We are to decide first whether we'll do
it or not.

[1] https://issues.apache.org/jira/browse/IGNITE-8227
<https://issues.apache.org/jira/browse/IGNITE-8227?focusedCommentId=16435298&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16435298>

чт, 20 сент. 2018 г. в 21:18, Dmitriy Pavlov <[hidden email]>:

> Sorry, incomplete message.
>
> Why do you think there is no consensus?
>
> I have no clue what can be a reason for another approach.
> By default failure handler should fail all test.
>
> Failure handlers test will be always a minority of tests, so fail handler
> call is something abnormal.
>
> чт, 20 сент. 2018 г. в 21:15, Dmitriy Pavlov <[hidden email]>:
>
> > Why do you think there is no consensus?
> >
> > I have no clue that by default failure handler should fail all test.
> >
> > чт, 20 сент. 2018 г. в 21:10, Andrey Kuznetsov <[hidden email]>:
> >
> >> I've created [1] to address this.
> >>
> >> Dmitriy, I like your idea of creating special test-scope handler. But
> >> there
> >> is no consensus about it, so I don't want to rely on that potential
> >> handler
> >> right now. We can switch to it later, of course.
> >>
> >> [1] https://issues.apache.org/jira/browse/IGNITE-9660
> >>
> >> чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <[hidden email]>:
> >>
> >> > Andrey,
> >> >
> >> > I like your idea.
> >> >
> >> > After changing the default node failure handler to the new one we
> should
> >> > carefully review the whole new test failures. For instance, calling
> this
> >> > method in tests should not lead test to the node being stopped:
> >> >
> >> > <strong>FOR TEST ONLY!!!</strong>
> >> > TcpDiscoverySpi#simulateNodeFailure
> >> >
> >> > BTW, I would like to remove this method at all from production code.
> >> >
> >> > On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <[hidden email]>
> >> wrote:
> >> >
> >> > > But the totally ideal situation would be finding a way to fail the
> >> test
> >> > by
> >> > > default, not only stopping a node.
> >> > >
> >> > > Some time ago I've created
> >> > > https://issues.apache.org/jira/browse/IGNITE-8227 to
> >> > > find out a way to do so.
> >> > >
> >> > > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <[hidden email]
> >:
> >> > >
> >> > > > ++1
> >> > > >
> >> > > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <[hidden email]
> >:
> >> > > >
> >> > > >> Igniters,
> >> > > >>
> >> > > >> While running tests I see a lot of ignored critical failures
> >> caused by
> >> > > >> {{NoOpFailureHandler}} that we use by default. In some tests, of
> >> > cource,
> >> > > >> critical failures are the part of normal workflow, but in the
> >> majority
> >> > > of
> >> > > >> tests they indicate bugs. By using {{NoOpFailureHandler}} we just
> >> hide
> >> > > >> these bugs from ourselves.
> >> > > >>
> >> > > >> What do you think about changing default handler to
> >> > > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This
> >> could
> >> > be
> >> > > >> overridden in subclasses.
> >> > > >>
> >> > > >> --
> >> > > >> Best regards,
> >> > > >>   Andrey Kuznetsov.
> >> > > >>
> >> > > >
> >> > >
> >> > --
> >> > --
> >> > Maxim Muzafarov
> >> >
> >>
> >>
> >> --
> >> Best regards,
> >>   Andrey Kuznetsov.
> >>
> >
>

--
Best regards,
  Andrey Kuznetsov.

[Vladimir Ozerov]

Stop node handler is not very good choice. Some test will continue work as
usual even if some node failed. E.g. SQL queries with backups may continue
function in some cases, especially if these are test with REPLICATED cache.

New test-scope handler looks like a better candidate to me.

чт, 20 сент. 2018 г. в 21:22, Andrey Kuznetsov <[hidden email]>:

> I meant the first comment in [1]. We are to decide first whether we'll do
> it or not.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-8227
> <
> https://issues.apache.org/jira/browse/IGNITE-8227?focusedCommentId=16435298&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16435298
> >
>
> чт, 20 сент. 2018 г. в 21:18, Dmitriy Pavlov <[hidden email]>:
>
> > Sorry, incomplete message.
> >
> > Why do you think there is no consensus?
> >
> > I have no clue what can be a reason for another approach.
> > By default failure handler should fail all test.
> >
> > Failure handlers test will be always a minority of tests, so fail handler
> > call is something abnormal.
> >
> > чт, 20 сент. 2018 г. в 21:15, Dmitriy Pavlov <[hidden email]>:
> >
> > > Why do you think there is no consensus?
> > >
> > > I have no clue that by default failure handler should fail all test.
> > >
> > > чт, 20 сент. 2018 г. в 21:10, Andrey Kuznetsov <[hidden email]>:
> > >
> > >> I've created [1] to address this.
> > >>
> > >> Dmitriy, I like your idea of creating special test-scope handler. But
> > >> there
> > >> is no consensus about it, so I don't want to rely on that potential
> > >> handler
> > >> right now. We can switch to it later, of course.
> > >>
> > >> [1] https://issues.apache.org/jira/browse/IGNITE-9660
> > >>
> > >> чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <[hidden email]>:
> > >>
> > >> > Andrey,
> > >> >
> > >> > I like your idea.
> > >> >
> > >> > After changing the default node failure handler to the new one we
> > should
> > >> > carefully review the whole new test failures. For instance, calling
> > this
> > >> > method in tests should not lead test to the node being stopped:
> > >> >
> > >> > <strong>FOR TEST ONLY!!!</strong>
> > >> > TcpDiscoverySpi#simulateNodeFailure
> > >> >
> > >> > BTW, I would like to remove this method at all from production code.
> > >> >
> > >> > On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <[hidden email]>
> > >> wrote:
> > >> >
> > >> > > But the totally ideal situation would be finding a way to fail the
> > >> test
> > >> > by
> > >> > > default, not only stopping a node.
> > >> > >
> > >> > > Some time ago I've created
> > >> > > https://issues.apache.org/jira/browse/IGNITE-8227 to
> > >> > > find out a way to do so.
> > >> > >
> > >> > > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <
> [hidden email]
> > >:
> > >> > >
> > >> > > > ++1
> > >> > > >
> > >> > > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <
> [hidden email]
> > >:
> > >> > > >
> > >> > > >> Igniters,
> > >> > > >>
> > >> > > >> While running tests I see a lot of ignored critical failures
> > >> caused by
> > >> > > >> {{NoOpFailureHandler}} that we use by default. In some tests,
> of
> > >> > cource,
> > >> > > >> critical failures are the part of normal workflow, but in the
> > >> majority
> > >> > > of
> > >> > > >> tests they indicate bugs. By using {{NoOpFailureHandler}} we
> just
> > >> hide
> > >> > > >> these bugs from ourselves.
> > >> > > >>
> > >> > > >> What do you think about changing default handler to
> > >> > > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level? This
> > >> could
> > >> > be
> > >> > > >> overridden in subclasses.
> > >> > > >>
> > >> > > >> --
> > >> > > >> Best regards,
> > >> > > >>   Andrey Kuznetsov.
> > >> > > >>
> > >> > > >
> > >> > >
> > >> > --
> > >> > --
> > >> > Maxim Muzafarov
> > >> >
> > >>
> > >>
> > >> --
> > >> Best regards,
> > >>   Andrey Kuznetsov.
> > >>
> > >
> >
>
>
> --
> Best regards,
>   Andrey Kuznetsov.
>

[Andrey Kuznetsov]

Thanks to all for participating the discussion.

I've updated [1]: now it requires new handler from [2] for completion.

[1] https://issues.apache.org/jira/browse/IGNITE-9660
[2] https://issues.apache.org/jira/browse/IGNITE-8227

чт, 20 сент. 2018 г. в 21:56, Vladimir Ozerov <[hidden email]>:

> Stop node handler is not very good choice. Some test will continue work as
> usual even if some node failed. E.g. SQL queries with backups may continue
> function in some cases, especially if these are test with REPLICATED cache.
>
> New test-scope handler looks like a better candidate to me.
>
> чт, 20 сент. 2018 г. в 21:22, Andrey Kuznetsov <[hidden email]>:
>
> > I meant the first comment in [1]. We are to decide first whether we'll do
> > it or not.
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > <
> >
> https://issues.apache.org/jira/browse/IGNITE-8227?focusedCommentId=16435298&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16435298
> > >
> >
> > чт, 20 сент. 2018 г. в 21:18, Dmitriy Pavlov <[hidden email]>:
> >
> > > Sorry, incomplete message.
> > >
> > > Why do you think there is no consensus?
> > >
> > > I have no clue what can be a reason for another approach.
> > > By default failure handler should fail all test.
> > >
> > > Failure handlers test will be always a minority of tests, so fail
> handler
> > > call is something abnormal.
> > >
> > > чт, 20 сент. 2018 г. в 21:15, Dmitriy Pavlov <[hidden email]>:
> > >
> > > > Why do you think there is no consensus?
> > > >
> > > > I have no clue that by default failure handler should fail all test.
> > > >
> > > > чт, 20 сент. 2018 г. в 21:10, Andrey Kuznetsov <[hidden email]>:
> > > >
> > > >> I've created [1] to address this.
> > > >>
> > > >> Dmitriy, I like your idea of creating special test-scope handler.
> But
> > > >> there
> > > >> is no consensus about it, so I don't want to rely on that potential
> > > >> handler
> > > >> right now. We can switch to it later, of course.
> > > >>
> > > >> [1] https://issues.apache.org/jira/browse/IGNITE-9660
> > > >>
> > > >> чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <[hidden email]>:
> > > >>
> > > >> > Andrey,
> > > >> >
> > > >> > I like your idea.
> > > >> >
> > > >> > After changing the default node failure handler to the new one we
> > > should
> > > >> > carefully review the whole new test failures. For instance,
> calling
> > > this
> > > >> > method in tests should not lead test to the node being stopped:
> > > >> >
> > > >> > <strong>FOR TEST ONLY!!!</strong>
> > > >> > TcpDiscoverySpi#simulateNodeFailure
> > > >> >
> > > >> > BTW, I would like to remove this method at all from production
> code.
> > > >> >
> > > >> > On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <
> [hidden email]>
> > > >> wrote:
> > > >> >
> > > >> > > But the totally ideal situation would be finding a way to fail
> the
> > > >> test
> > > >> > by
> > > >> > > default, not only stopping a node.
> > > >> > >
> > > >> > > Some time ago I've created
> > > >> > > https://issues.apache.org/jira/browse/IGNITE-8227 to
> > > >> > > find out a way to do so.
> > > >> > >
> > > >> > > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <
> > [hidden email]
> > > >:
> > > >> > >
> > > >> > > > ++1
> > > >> > > >
> > > >> > > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <
> > [hidden email]
> > > >:
> > > >> > > >
> > > >> > > >> Igniters,
> > > >> > > >>
> > > >> > > >> While running tests I see a lot of ignored critical failures
> > > >> caused by
> > > >> > > >> {{NoOpFailureHandler}} that we use by default. In some tests,
> > of
> > > >> > cource,
> > > >> > > >> critical failures are the part of normal workflow, but in the
> > > >> majority
> > > >> > > of
> > > >> > > >> tests they indicate bugs. By using {{NoOpFailureHandler}} we
> > just
> > > >> hide
> > > >> > > >> these bugs from ourselves.
> > > >> > > >>
> > > >> > > >> What do you think about changing default handler to
> > > >> > > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level?
> This
> > > >> could
> > > >> > be
> > > >> > > >> overridden in subclasses.
> > > >> > > >>
> > > >> > > >> --
> > > >> > > >> Best regards,
> > > >> > > >>   Andrey Kuznetsov.
> > > >> > > >>
> > > >> > > >
> > > >> > >
> > > >> > --
> > > >> > --
> > > >> > Maxim Muzafarov
> > > >> >
> > > >>
> > > >>
> > > >> --
> > > >> Best regards,
> > > >>   Andrey Kuznetsov.
> > > >>
> > > >
> > >
> >
> >
> > --
> > Best regards,
> >   Andrey Kuznetsov.
> >
>

--
Best regards,
  Andrey Kuznetsov.

[Dmitrii Ryabov]

I tried to replace default no-op handler by handler stopping node and
failing the test.

I've returned the no-op handler in many classes because critical
situations are expected behavior. But PR still have a lot of failed
tests and suites. In some tests, I can't understand a failure reason.

I'm not finished to check failures, but after several RunAll runs, I
see new flaky tests (1 or 2 fails) appeared because of new handler.

I think we should keep no-op handler as default, but add new handler
for a few classes, where critical situations aren't expected.

пт, 21 сент. 2018 г. в 17:03, Andrey Kuznetsov <[hidden email]>:

>
> Thanks to all for participating the discussion.
>
> I've updated [1]: now it requires new handler from [2] for completion.
>
> [1] https://issues.apache.org/jira/browse/IGNITE-9660
> [2] https://issues.apache.org/jira/browse/IGNITE-8227
>
> чт, 20 сент. 2018 г. в 21:56, Vladimir Ozerov <[hidden email]>:
>
> > Stop node handler is not very good choice. Some test will continue work as
> > usual even if some node failed. E.g. SQL queries with backups may continue
> > function in some cases, especially if these are test with REPLICATED cache.
> >
> > New test-scope handler looks like a better candidate to me.
> >
> > чт, 20 сент. 2018 г. в 21:22, Andrey Kuznetsov <[hidden email]>:
> >
> > > I meant the first comment in [1]. We are to decide first whether we'll do
> > > it or not.
> > >
> > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > <
> > >
> > https://issues.apache.org/jira/browse/IGNITE-8227?focusedCommentId=16435298&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16435298
> > > >
> > >
> > > чт, 20 сент. 2018 г. в 21:18, Dmitriy Pavlov <[hidden email]>:
> > >
> > > > Sorry, incomplete message.
> > > >
> > > > Why do you think there is no consensus?
> > > >
> > > > I have no clue what can be a reason for another approach.
> > > > By default failure handler should fail all test.
> > > >
> > > > Failure handlers test will be always a minority of tests, so fail
> > handler
> > > > call is something abnormal.
> > > >
> > > > чт, 20 сент. 2018 г. в 21:15, Dmitriy Pavlov <[hidden email]>:
> > > >
> > > > > Why do you think there is no consensus?
> > > > >
> > > > > I have no clue that by default failure handler should fail all test.
> > > > >
> > > > > чт, 20 сент. 2018 г. в 21:10, Andrey Kuznetsov <[hidden email]>:
> > > > >
> > > > >> I've created [1] to address this.
> > > > >>
> > > > >> Dmitriy, I like your idea of creating special test-scope handler.
> > But
> > > > >> there
> > > > >> is no consensus about it, so I don't want to rely on that potential
> > > > >> handler
> > > > >> right now. We can switch to it later, of course.
> > > > >>
> > > > >> [1] https://issues.apache.org/jira/browse/IGNITE-9660
> > > > >>
> > > > >> чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <[hidden email]>:
> > > > >>
> > > > >> > Andrey,
> > > > >> >
> > > > >> > I like your idea.
> > > > >> >
> > > > >> > After changing the default node failure handler to the new one we
> > > > should
> > > > >> > carefully review the whole new test failures. For instance,
> > calling
> > > > this
> > > > >> > method in tests should not lead test to the node being stopped:
> > > > >> >
> > > > >> > <strong>FOR TEST ONLY!!!</strong>
> > > > >> > TcpDiscoverySpi#simulateNodeFailure
> > > > >> >
> > > > >> > BTW, I would like to remove this method at all from production
> > code.
> > > > >> >
> > > > >> > On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <
> > [hidden email]>
> > > > >> wrote:
> > > > >> >
> > > > >> > > But the totally ideal situation would be finding a way to fail
> > the
> > > > >> test
> > > > >> > by
> > > > >> > > default, not only stopping a node.
> > > > >> > >
> > > > >> > > Some time ago I've created
> > > > >> > > https://issues.apache.org/jira/browse/IGNITE-8227 to
> > > > >> > > find out a way to do so.
> > > > >> > >
> > > > >> > > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <
> > > [hidden email]
> > > > >:
> > > > >> > >
> > > > >> > > > ++1
> > > > >> > > >
> > > > >> > > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <
> > > [hidden email]
> > > > >:
> > > > >> > > >
> > > > >> > > >> Igniters,
> > > > >> > > >>
> > > > >> > > >> While running tests I see a lot of ignored critical failures
> > > > >> caused by
> > > > >> > > >> {{NoOpFailureHandler}} that we use by default. In some tests,
> > > of
> > > > >> > cource,
> > > > >> > > >> critical failures are the part of normal workflow, but in the
> > > > >> majority
> > > > >> > > of
> > > > >> > > >> tests they indicate bugs. By using {{NoOpFailureHandler}} we
> > > just
> > > > >> hide
> > > > >> > > >> these bugs from ourselves.
> > > > >> > > >>
> > > > >> > > >> What do you think about changing default handler to
> > > > >> > > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level?
> > This
> > > > >> could
> > > > >> > be
> > > > >> > > >> overridden in subclasses.
> > > > >> > > >>
> > > > >> > > >> --
> > > > >> > > >> Best regards,
> > > > >> > > >>   Andrey Kuznetsov.
> > > > >> > > >>
> > > > >> > > >
> > > > >> > >
> > > > >> > --
> > > > >> > --
> > > > >> > Maxim Muzafarov
> > > > >> >
> > > > >>
> > > > >>
> > > > >> --
> > > > >> Best regards,
> > > > >>   Andrey Kuznetsov.
> > > > >>
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best regards,
> > >   Andrey Kuznetsov.
> > >
> >
>
>
> --
> Best regards,
>   Andrey Kuznetsov.

[Andrey Kuznetsov]

Hi, Dmitrii!

As for [1], I think your suggestion is good to complete the change.

As for [2], I tend to disagree: in future tests default no-op handler can
hide bugs of new Ignite functionality. If it is really needed, developer
can explicitly mention that critical failure is a normal part of test
operation.

[1] https://issues.apache.org/jira/browse/IGNITE-8227
[2] https://issues.apache.org/jira/browse/IGNITE-9660

пн, 22 окт. 2018 г. в 15:18, Dmitrii Ryabov <[hidden email]>:

> I tried to replace default no-op handler by handler stopping node and
> failing the test.
>
> I've returned the no-op handler in many classes because critical
> situations are expected behavior. But PR still have a lot of failed
> tests and suites. In some tests, I can't understand a failure reason.
>
> I'm not finished to check failures, but after several RunAll runs, I
> see new flaky tests (1 or 2 fails) appeared because of new handler.
>
> I think we should keep no-op handler as default, but add new handler
> for a few classes, where critical situations aren't expected.
>
> пт, 21 сент. 2018 г. в 17:03, Andrey Kuznetsov <[hidden email]>:
> >
> > Thanks to all for participating the discussion.
> >
> > I've updated [1]: now it requires new handler from [2] for completion.
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-9660
> > [2] https://issues.apache.org/jira/browse/IGNITE-8227
> >
> > чт, 20 сент. 2018 г. в 21:56, Vladimir Ozerov <[hidden email]>:
> >
> > > Stop node handler is not very good choice. Some test will continue
> work as
> > > usual even if some node failed. E.g. SQL queries with backups may
> continue
> > > function in some cases, especially if these are test with REPLICATED
> cache.
> > >
> > > New test-scope handler looks like a better candidate to me.
> > >
> > > чт, 20 сент. 2018 г. в 21:22, Andrey Kuznetsov <[hidden email]>:
> > >
> > > > I meant the first comment in [1]. We are to decide first whether
> we'll do
> > > > it or not.
> > > >
> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > > <
> > > >
> > >
> https://issues.apache.org/jira/browse/IGNITE-8227?focusedCommentId=16435298&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16435298
> > > > >
> > > >
> > > > чт, 20 сент. 2018 г. в 21:18, Dmitriy Pavlov <[hidden email]
> >:
> > > >
> > > > > Sorry, incomplete message.
> > > > >
> > > > > Why do you think there is no consensus?
> > > > >
> > > > > I have no clue what can be a reason for another approach.
> > > > > By default failure handler should fail all test.
> > > > >
> > > > > Failure handlers test will be always a minority of tests, so fail
> > > handler
> > > > > call is something abnormal.
> > > > >
> > > > > чт, 20 сент. 2018 г. в 21:15, Dmitriy Pavlov <
> [hidden email]>:
> > > > >
> > > > > > Why do you think there is no consensus?
> > > > > >
> > > > > > I have no clue that by default failure handler should fail all
> test.
> > > > > >
> > > > > > чт, 20 сент. 2018 г. в 21:10, Andrey Kuznetsov <
> [hidden email]>:
> > > > > >
> > > > > >> I've created [1] to address this.
> > > > > >>
> > > > > >> Dmitriy, I like your idea of creating special test-scope
> handler.
> > > But
> > > > > >> there
> > > > > >> is no consensus about it, so I don't want to rely on that
> potential
> > > > > >> handler
> > > > > >> right now. We can switch to it later, of course.
> > > > > >>
> > > > > >> [1] https://issues.apache.org/jira/browse/IGNITE-9660
> > > > > >>
> > > > > >> чт, 20 сент. 2018 г. в 20:03, Maxim Muzafarov <
> [hidden email]>:
> > > > > >>
> > > > > >> > Andrey,
> > > > > >> >
> > > > > >> > I like your idea.
> > > > > >> >
> > > > > >> > After changing the default node failure handler to the new
> one we
> > > > > should
> > > > > >> > carefully review the whole new test failures. For instance,
> > > calling
> > > > > this
> > > > > >> > method in tests should not lead test to the node being
> stopped:
> > > > > >> >
> > > > > >> > <strong>FOR TEST ONLY!!!</strong>
> > > > > >> > TcpDiscoverySpi#simulateNodeFailure
> > > > > >> >
> > > > > >> > BTW, I would like to remove this method at all from production
> > > code.
> > > > > >> >
> > > > > >> > On Thu, 20 Sep 2018 at 19:43 Dmitriy Pavlov <
> > > [hidden email]>
> > > > > >> wrote:
> > > > > >> >
> > > > > >> > > But the totally ideal situation would be finding a way to
> fail
> > > the
> > > > > >> test
> > > > > >> > by
> > > > > >> > > default, not only stopping a node.
> > > > > >> > >
> > > > > >> > > Some time ago I've created
> > > > > >> > > https://issues.apache.org/jira/browse/IGNITE-8227 to
> > > > > >> > > find out a way to do so.
> > > > > >> > >
> > > > > >> > > чт, 20 сент. 2018 г. в 19:40, Dmitriy Pavlov <
> > > > [hidden email]
> > > > > >:
> > > > > >> > >
> > > > > >> > > > ++1
> > > > > >> > > >
> > > > > >> > > > чт, 20 сент. 2018 г. в 19:39, Andrey Kuznetsov <
> > > > [hidden email]
> > > > > >:
> > > > > >> > > >
> > > > > >> > > >> Igniters,
> > > > > >> > > >>
> > > > > >> > > >> While running tests I see a lot of ignored critical
> failures
> > > > > >> caused by
> > > > > >> > > >> {{NoOpFailureHandler}} that we use by default. In some
> tests,
> > > > of
> > > > > >> > cource,
> > > > > >> > > >> critical failures are the part of normal workflow, but
> in the
> > > > > >> majority
> > > > > >> > > of
> > > > > >> > > >> tests they indicate bugs. By using
> {{NoOpFailureHandler}} we
> > > > just
> > > > > >> hide
> > > > > >> > > >> these bugs from ourselves.
> > > > > >> > > >>
> > > > > >> > > >> What do you think about changing default handler to
> > > > > >> > > >> {{StopNodeFailureHandler}} at {{GridAbstractTest}} level?
> > > This
> > > > > >> could
> > > > > >> > be
> > > > > >> > > >> overridden in subclasses.
> > > > > >> > > >>
> > > > > >> > > >> --
> > > > > >> > > >> Best regards,
> > > > > >> > > >>   Andrey Kuznetsov.
> > > > > >> > > >>
> > > > > >> > > >
> > > > > >> > >
> > > > > >> > --
> > > > > >> > --
> > > > > >> > Maxim Muzafarov
> > > > > >> >
> > > > > >>
> > > > > >>
> > > > > >> --
> > > > > >> Best regards,
> > > > > >>   Andrey Kuznetsov.
> > > > > >>
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > >   Andrey Kuznetsov.
> > > >
> > >
> >
> >
> > --
> > Best regards,
> >   Andrey Kuznetsov.
>

--
Best regards,
  Andrey Kuznetsov.