Apache Ignite Developers - Legacy Mail Archive

Default failure handler was changed for tests

Classic

List

Threaded

64 messages Options

1234

Dmitrii Ryabov

Default failure handler was changed for tests

Hello, Igniters!

Today the test framework's default no-op failure handler was changed to the
handler, which stops the node and fails the test.

Over 100 tests kept no-op failure handler by overrided
`getFailureHandler()` method.

If you'll found a problem or something unexpected - write here or in the
ticket [1].

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

Anton Vinogradov-2

Re: Default failure handler was changed for tests

Dmitrii,

Could you please explain the reason of explicit set of 100+
NoOpFailureHandlers?

вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <[hidden email]>:

> Hello, Igniters!
>
> Today the test framework's default no-op failure handler was changed to the
> handler, which stops the node and fails the test.
>
> Over 100 tests kept no-op failure handler by overrided
> `getFailureHandler()` method.
>
> If you'll found a problem or something unexpected - write here or in the
> ticket [1].
>
> [1] https://issues.apache.org/jira/browse/IGNITE-8227
>

Dmitry Pavlov

Re: Default failure handler was changed for tests

Hi Igniters,

BTW, if you find in any of your tests it does't need an old value of
handler (=NoOp), feel free to remove it.

Sincerely,
Dmitriy Pavlov

вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:

> Dmitrii,
>
> Could you please explain the reason of explicit set of 100+
> NoOpFailureHandlers?
>
>
> вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <[hidden email]>:
>
> > Hello, Igniters!
> >
> > Today the test framework's default no-op failure handler was changed to
> the
> > handler, which stops the node and fails the test.
> >
> > Over 100 tests kept no-op failure handler by overrided
> > `getFailureHandler()` method.
> >
> > If you'll found a problem or something unexpected - write here or in the
> > ticket [1].
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> >
>

Dmitrii Ryabov

Re: Default failure handler was changed for tests

Anton,

Tests in these classes check fail cases when we expect critical
failure like node stop or exception thrown. Such tests trigger failure
handler and it fails test when everything goes as it should go. That's
why we need no-op handler here.
вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]>:

>
> Hi Igniters,
>
> BTW, if you find in any of your tests it does't need an old value of
> handler (=NoOp), feel free to remove it.
>
> Sincerely,
> Dmitriy Pavlov
>
> вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
>
> > Dmitrii,
> >
> > Could you please explain the reason of explicit set of 100+
> > NoOpFailureHandlers?
> >
> >
> > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <[hidden email]>:
> >
> > > Hello, Igniters!
> > >
> > > Today the test framework's default no-op failure handler was changed to
> > the
> > > handler, which stops the node and fails the test.
> > >
> > > Over 100 tests kept no-op failure handler by overrided
> > > `getFailureHandler()` method.
> > >
> > > If you'll found a problem or something unexpected - write here or in the
> > > ticket [1].
> > >
> > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > >
> >

Anton Vinogradov-2

Re: Default failure handler was changed for tests

Dmitrii,

The solution is not clear to me.
In case you expect the failure then a correct case is to wrap it with
try-catch block instead of no-op failure handler usage.

вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <[hidden email]>:

> Anton,
>
> Tests in these classes check fail cases when we expect critical
> failure like node stop or exception thrown. Such tests trigger failure
> handler and it fails test when everything goes as it should go. That's
> why we need no-op handler here.
> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]>:
> >
> > Hi Igniters,
> >
> > BTW, if you find in any of your tests it does't need an old value of
> > handler (=NoOp), feel free to remove it.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
> >
> > > Dmitrii,
> > >
> > > Could you please explain the reason of explicit set of 100+
> > > NoOpFailureHandlers?
> > >
> > >
> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <[hidden email]>:
> > >
> > > > Hello, Igniters!
> > > >
> > > > Today the test framework's default no-op failure handler was changed
> to
> > > the
> > > > handler, which stops the node and fails the test.
> > > >
> > > > Over 100 tests kept no-op failure handler by overrided
> > > > `getFailureHandler()` method.
> > > >
> > > > If you'll found a problem or something unexpected - write here or in
> the
> > > > ticket [1].
> > > >
> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > >
> > >
>

Anton Vinogradov-2

Re: Default failure handler was changed for tests

And you have to check the reason of failure inside the try-catch block, of
course.
In case found not equals to expected then test should rethrow the exception.

вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:

> Dmitrii,
>
> The solution is not clear to me.
> In case you expect the failure then a correct case is to wrap it with
> try-catch block instead of no-op failure handler usage.
>
> вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <[hidden email]>:
>
>> Anton,
>>
>> Tests in these classes check fail cases when we expect critical
>> failure like node stop or exception thrown. Such tests trigger failure
>> handler and it fails test when everything goes as it should go. That's
>> why we need no-op handler here.
>> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]>:
>> >
>> > Hi Igniters,
>> >
>> > BTW, if you find in any of your tests it does't need an old value of
>> > handler (=NoOp), feel free to remove it.
>> >
>> > Sincerely,
>> > Dmitriy Pavlov
>> >
>> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
>> >
>> > > Dmitrii,
>> > >
>> > > Could you please explain the reason of explicit set of 100+
>> > > NoOpFailureHandlers?
>> > >
>> > >
>> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <[hidden email]>:
>> > >
>> > > > Hello, Igniters!
>> > > >
>> > > > Today the test framework's default no-op failure handler was
>> changed to
>> > > the
>> > > > handler, which stops the node and fails the test.
>> > > >
>> > > > Over 100 tests kept no-op failure handler by overrided
>> > > > `getFailureHandler()` method.
>> > > >
>> > > > If you'll found a problem or something unexpected - write here or
>> in the
>> > > > ticket [1].
>> > > >
>> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
>> > > >
>> > >
>>
>

Andrew Mashenkov

Re: Default failure handler was changed for tests

Hi all,

Really, why noop?

If you expect failure handler should be triggered, you can override default
one and rise some flag, which can be checked in test.
This will make test clearer.

With noop, you'll get previous unwanted behavior, that you are trying to
improve, isnt'it?

4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <[hidden email]>
написал:

And you have to check the reason of failure inside the try-catch block, of
course.
In case found not equals to expected then test should rethrow the exception.

вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:

Dmitry Pavlov

Re: Default failure handler was changed for tests

Folks let me remind you that Dmitry changed default of ALL tests from noop
to a meaningful handler. So we should start every message here from saying
thank you to Dmitry.

Please review remaining tests and remove noop where possible.

вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <[hidden email]>:

> Hi all,
>
> Really, why noop?
>
> If you expect failure handler should be triggered, you can override default
> one and rise some flag, which can be checked in test.
> This will make test clearer.
>
> With noop, you'll get previous unwanted behavior, that you are trying to
> improve, isnt'it?
>
> 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <[hidden email]>
> написал:
>
> And you have to check the reason of failure inside the try-catch block, of
> course.
> In case found not equals to expected then test should rethrow the
> exception.
>
>
> вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:
>
>
> > Dmitrii,
> >
> > The solution is not clear to me.
> > In case you expect the failure then a correct case is to wrap it with
> > try-catch block instead of no-op failure handler usage.
> >
> > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <[hidden email]>:
> >
> >> Anton,
> >>
> >> Tests in these classes check fail cases when we expect critical
> >> failure like node stop or exception thrown. Such tests trigger failure
> >> handler and it fails test when everything goes as it should go. That's
> >> why we need no-op handler here.
> >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]>:
> >> >
> >> > Hi Igniters,
> >> >
> >> > BTW, if you find in any of your tests it does't need an old value of
> >> > handler (=NoOp), feel free to remove it.
> >> >
> >> > Sincerely,
> >> > Dmitriy Pavlov
> >> >
> >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
> >> >
> >> > > Dmitrii,
> >> > >
> >> > > Could you please explain the reason of explicit set of 100+
> >> > > NoOpFailureHandlers?
> >> > >
> >> > >
> >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <[hidden email]>:
> >> > >
> >> > > > Hello, Igniters!
> >> > > >
> >> > > > Today the test framework's default no-op failure handler was
> >> changed to
> >> > > the
> >> > > > handler, which stops the node and fails the test.
> >> > > >
> >> > > > Over 100 tests kept no-op failure handler by overrided
> >> > > > `getFailureHandler()` method.
> >> > > >
> >> > > > If you'll found a problem or something unexpected - write here or
> >> in the
> >> > > > ticket [1].
> >> > > >
> >> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> >> > > >
> >> > >
> >>
> >
>

Dmitrii Ryabov

Re: Default failure handler was changed for tests

Anton, I think wrapping every disconnecting node with try-catch will be
less readable than no-op handler.

ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:

> Folks let me remind you that Dmitry changed default of ALL tests from noop
> to a meaningful handler. So we should start every message here from saying
> thank you to Dmitry.
>
> Please review remaining tests and remove noop where possible.
>
> вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <[hidden email]>:
>
> > Hi all,
> >
> > Really, why noop?
> >
> > If you expect failure handler should be triggered, you can override
> default
> > one and rise some flag, which can be checked in test.
> > This will make test clearer.
> >
> > With noop, you'll get previous unwanted behavior, that you are trying to
> > improve, isnt'it?
> >
> > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <[hidden email]>
> > написал:
> >
> > And you have to check the reason of failure inside the try-catch block,
> of
> > course.
> > In case found not equals to expected then test should rethrow the
> > exception.
> >
> >
> > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:
> >
> >
> > > Dmitrii,
> > >
> > > The solution is not clear to me.
> > > In case you expect the failure then a correct case is to wrap it with
> > > try-catch block instead of no-op failure handler usage.
> > >
> > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <[hidden email]>:
> > >
> > >> Anton,
> > >>
> > >> Tests in these classes check fail cases when we expect critical
> > >> failure like node stop or exception thrown. Such tests trigger failure
> > >> handler and it fails test when everything goes as it should go. That's
> > >> why we need no-op handler here.
> > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]>:
> > >> >
> > >> > Hi Igniters,
> > >> >
> > >> > BTW, if you find in any of your tests it does't need an old value of
> > >> > handler (=NoOp), feel free to remove it.
> > >> >
> > >> > Sincerely,
> > >> > Dmitriy Pavlov
> > >> >
> > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
> > >> >
> > >> > > Dmitrii,
> > >> > >
> > >> > > Could you please explain the reason of explicit set of 100+
> > >> > > NoOpFailureHandlers?
> > >> > >
> > >> > >
> > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <[hidden email]
> >:
> > >> > >
> > >> > > > Hello, Igniters!
> > >> > > >
> > >> > > > Today the test framework's default no-op failure handler was
> > >> changed to
> > >> > > the
> > >> > > > handler, which stops the node and fails the test.
> > >> > > >
> > >> > > > Over 100 tests kept no-op failure handler by overrided
> > >> > > > `getFailureHandler()` method.
> > >> > > >
> > >> > > > If you'll found a problem or something unexpected - write here
> or
> > >> in the
> > >> > > > ticket [1].
> > >> > > >
> > >> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > >> > > >
> > >> > >
> > >>
> > >
> >
>

Anton Vinogradov-2

Re: Default failure handler was changed for tests

Dmitrii,

No-op means "hide any problem", so, we lose the guarantees.
Could you please share some examples where "no-op" better than "strict
try-catch with a check"?

On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <[hidden email]>
wrote:

> Anton, I think wrapping every disconnecting node with try-catch will be
> less readable than no-op handler.
>
> ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
>
> > Folks let me remind you that Dmitry changed default of ALL tests from
> noop
> > to a meaningful handler. So we should start every message here from
> saying
> > thank you to Dmitry.
> >
> > Please review remaining tests and remove noop where possible.
> >
> > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <[hidden email]>:
> >
> > > Hi all,
> > >
> > > Really, why noop?
> > >
> > > If you expect failure handler should be triggered, you can override
> > default
> > > one and rise some flag, which can be checked in test.
> > > This will make test clearer.
> > >
> > > With noop, you'll get previous unwanted behavior, that you are trying
> to
> > > improve, isnt'it?
> > >
> > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <[hidden email]>
> > > написал:
> > >
> > > And you have to check the reason of failure inside the try-catch block,
> > of
> > > course.
> > > In case found not equals to expected then test should rethrow the
> > > exception.
> > >
> > >
> > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:
> > >
> > >
> > > > Dmitrii,
> > > >
> > > > The solution is not clear to me.
> > > > In case you expect the failure then a correct case is to wrap it with
> > > > try-catch block instead of no-op failure handler usage.
> > > >
> > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <[hidden email]>:
> > > >
> > > >> Anton,
> > > >>
> > > >> Tests in these classes check fail cases when we expect critical
> > > >> failure like node stop or exception thrown. Such tests trigger
> failure
> > > >> handler and it fails test when everything goes as it should go.
> That's
> > > >> why we need no-op handler here.
> > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]>:
> > > >> >
> > > >> > Hi Igniters,
> > > >> >
> > > >> > BTW, if you find in any of your tests it does't need an old value
> of
> > > >> > handler (=NoOp), feel free to remove it.
> > > >> >
> > > >> > Sincerely,
> > > >> > Dmitriy Pavlov
> > > >> >
> > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
> > > >> >
> > > >> > > Dmitrii,
> > > >> > >
> > > >> > > Could you please explain the reason of explicit set of 100+
> > > >> > > NoOpFailureHandlers?
> > > >> > >
> > > >> > >
> > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> [hidden email]
> > >:
> > > >> > >
> > > >> > > > Hello, Igniters!
> > > >> > > >
> > > >> > > > Today the test framework's default no-op failure handler was
> > > >> changed to
> > > >> > > the
> > > >> > > > handler, which stops the node and fails the test.
> > > >> > > >
> > > >> > > > Over 100 tests kept no-op failure handler by overrided
> > > >> > > > `getFailureHandler()` method.
> > > >> > > >
> > > >> > > > If you'll found a problem or something unexpected - write here
> > or
> > > >> in the
> > > >> > > > ticket [1].
> > > >> > > >
> > > >> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > >> > > >
> > > >> > >
> > > >>
> > > >
> > >
> >
>

Dmitry Pavlov

Re: Default failure handler was changed for tests

Anton,

If I understood this idea right, try-catch will not work because failure
can be thrown into an Ignite thread pool, which catches any exceptions and
errors.

Which code block will do a throw?

Sincerely,
Dmitriy Pavlov

ср, 5 дек. 2018 г. в 12:16, Anton Vinogradov <[hidden email]>:

> Dmitrii,
>
> No-op means "hide any problem", so, we lose the guarantees.
> Could you please share some examples where "no-op" better than "strict
> try-catch with a check"?
>
> On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <[hidden email]>
> wrote:
>
> > Anton, I think wrapping every disconnecting node with try-catch will be
> > less readable than no-op handler.
> >
> > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
> >
> > > Folks let me remind you that Dmitry changed default of ALL tests from
> > noop
> > > to a meaningful handler. So we should start every message here from
> > saying
> > > thank you to Dmitry.
> > >
> > > Please review remaining tests and remove noop where possible.
> > >
> > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <[hidden email]
> >:
> > >
> > > > Hi all,
> > > >
> > > > Really, why noop?
> > > >
> > > > If you expect failure handler should be triggered, you can override
> > > default
> > > > one and rise some flag, which can be checked in test.
> > > > This will make test clearer.
> > > >
> > > > With noop, you'll get previous unwanted behavior, that you are
> trying
> > to
> > > > improve, isnt'it?
> > > >
> > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <[hidden email]>
> > > > написал:
> > > >
> > > > And you have to check the reason of failure inside the try-catch
> block,
> > > of
> > > > course.
> > > > In case found not equals to expected then test should rethrow the
> > > > exception.
> > > >
> > > >
> > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:
> > > >
> > > >
> > > > > Dmitrii,
> > > > >
> > > > > The solution is not clear to me.
> > > > > In case you expect the failure then a correct case is to wrap it
> with
> > > > > try-catch block instead of no-op failure handler usage.
> > > > >
> > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <[hidden email]
> >:
> > > > >
> > > > >> Anton,
> > > > >>
> > > > >> Tests in these classes check fail cases when we expect critical
> > > > >> failure like node stop or exception thrown. Such tests trigger
> > failure
> > > > >> handler and it fails test when everything goes as it should go.
> > That's
> > > > >> why we need no-op handler here.
> > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]>:
> > > > >> >
> > > > >> > Hi Igniters,
> > > > >> >
> > > > >> > BTW, if you find in any of your tests it does't need an old
> value
> > of
> > > > >> > handler (=NoOp), feel free to remove it.
> > > > >> >
> > > > >> > Sincerely,
> > > > >> > Dmitriy Pavlov
> > > > >> >
> > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
> > > > >> >
> > > > >> > > Dmitrii,
> > > > >> > >
> > > > >> > > Could you please explain the reason of explicit set of 100+
> > > > >> > > NoOpFailureHandlers?
> > > > >> > >
> > > > >> > >
> > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > [hidden email]
> > > >:
> > > > >> > >
> > > > >> > > > Hello, Igniters!
> > > > >> > > >
> > > > >> > > > Today the test framework's default no-op failure handler was
> > > > >> changed to
> > > > >> > > the
> > > > >> > > > handler, which stops the node and fails the test.
> > > > >> > > >
> > > > >> > > > Over 100 tests kept no-op failure handler by overrided
> > > > >> > > > `getFailureHandler()` method.
> > > > >> > > >
> > > > >> > > > If you'll found a problem or something unexpected - write
> here
> > > or
> > > > >> in the
> > > > >> > > > ticket [1].
> > > > >> > > >
> > > > >> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > > >> > > >
> > > > >> > >
> > > > >>
> > > > >
> > > >
> > >
> >
>

Andrew Mashenkov

Re: Default failure handler was changed for tests

In reply to this post by Anton Vinogradov-2

Hi,

Dmitri,

The meaningful failure handler as a default one looks reasonable.
Thanks a lot.

But what is the reason to fallback to noop for 100+ test?
Does it means these test become failed after changing default failure
handler?
If so, let's create a ticket (may be umbrella) to investigate and fix this.

I see 100+ touched files in PR and some of them are abstract classes, so,
we have much more affected tests.
Seems, most of failover test doesn't expects if any critical internal issue
occur and there is no need to fallback to noop.
Other test should set custom failure handler to detect expected failures or
if grid hanging simulation is needed (to keep hanged grid under control).

On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <[hidden email]> wrote:

--
Best regards,
Andrey V. Mashenkov

Anton Vinogradov-2

Re: Default failure handler was changed for tests

Dmitriy,

>> Which code block will do a throw?
Depends on the test.

Looks like we make the *bad *test even *worse*.

That's not a correct fix.
In case you expect failure you have to check this expectation inside the
special handler.

I'd like to ask you to rollback these changes and replace them with correct
fixes.

On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <[hidden email]>
wrote:

> Hi,
>
> Dmitri,
>
> The meaningful failure handler as a default one looks reasonable.
> Thanks a lot.
>
> But what is the reason to fallback to noop for 100+ test?
> Does it means these test become failed after changing default failure
> handler?
> If so, let's create a ticket (may be umbrella) to investigate and fix this.
>
> I see 100+ touched files in PR and some of them are abstract classes, so,
> we have much more affected tests.
> Seems, most of failover test doesn't expects if any critical internal issue
> occur and there is no need to fallback to noop.
> Other test should set custom failure handler to detect expected failures or
> if grid hanging simulation is needed (to keep hanged grid under control).
>
> On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <[hidden email]> wrote:
>
> > Dmitrii,
> >
> > No-op means "hide any problem", so, we lose the guarantees.
> > Could you please share some examples where "no-op" better than "strict
> > try-catch with a check"?
> >
> > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <[hidden email]>
> > wrote:
> >
> > > Anton, I think wrapping every disconnecting node with try-catch will be
> > > less readable than no-op handler.
> > >
> > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
> > >
> > > > Folks let me remind you that Dmitry changed default of ALL tests from
> > > noop
> > > > to a meaningful handler. So we should start every message here from
> > > saying
> > > > thank you to Dmitry.
> > > >
> > > > Please review remaining tests and remove noop where possible.
> > > >
> > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> [hidden email]
> > >:
> > > >
> > > > > Hi all,
> > > > >
> > > > > Really, why noop?
> > > > >
> > > > > If you expect failure handler should be triggered, you can override
> > > > default
> > > > > one and rise some flag, which can be checked in test.
> > > > > This will make test clearer.
> > > > >
> > > > > With noop, you'll get previous unwanted behavior, that you are
> > trying
> > > to
> > > > > improve, isnt'it?
> > > > >
> > > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <
> [hidden email]>
> > > > > написал:
> > > > >
> > > > > And you have to check the reason of failure inside the try-catch
> > block,
> > > > of
> > > > > course.
> > > > > In case found not equals to expected then test should rethrow the
> > > > > exception.
> > > > >
> > > > >
> > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:
> > > > >
> > > > >
> > > > > > Dmitrii,
> > > > > >
> > > > > > The solution is not clear to me.
> > > > > > In case you expect the failure then a correct case is to wrap it
> > with
> > > > > > try-catch block instead of no-op failure handler usage.
> > > > > >
> > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <
> [hidden email]
> > >:
> > > > > >
> > > > > >> Anton,
> > > > > >>
> > > > > >> Tests in these classes check fail cases when we expect critical
> > > > > >> failure like node stop or exception thrown. Such tests trigger
> > > failure
> > > > > >> handler and it fails test when everything goes as it should go.
> > > That's
> > > > > >> why we need no-op handler here.
> > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <[hidden email]
> >:
> > > > > >> >
> > > > > >> > Hi Igniters,
> > > > > >> >
> > > > > >> > BTW, if you find in any of your tests it does't need an old
> > value
> > > of
> > > > > >> > handler (=NoOp), feel free to remove it.
> > > > > >> >
> > > > > >> > Sincerely,
> > > > > >> > Dmitriy Pavlov
> > > > > >> >
> > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]>:
> > > > > >> >
> > > > > >> > > Dmitrii,
> > > > > >> > >
> > > > > >> > > Could you please explain the reason of explicit set of 100+
> > > > > >> > > NoOpFailureHandlers?
> > > > > >> > >
> > > > > >> > >
> > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > > [hidden email]
> > > > >:
> > > > > >> > >
> > > > > >> > > > Hello, Igniters!
> > > > > >> > > >
> > > > > >> > > > Today the test framework's default no-op failure handler
> was
> > > > > >> changed to
> > > > > >> > > the
> > > > > >> > > > handler, which stops the node and fails the test.
> > > > > >> > > >
> > > > > >> > > > Over 100 tests kept no-op failure handler by overrided
> > > > > >> > > > `getFailureHandler()` method.
> > > > > >> > > >
> > > > > >> > > > If you'll found a problem or something unexpected - write
> > here
> > > > or
> > > > > >> in the
> > > > > >> > > > ticket [1].
> > > > > >> > > >
> > > > > >> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > >> > > >
> > > > > >> > >
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
> --
> Best regards,
> Andrey V. Mashenkov
>

Dmitry Pavlov

Re: Default failure handler was changed for tests

I will not do any rollback because changes make tests better. Please pay
attention that no-op became default long time ago. Please discuss this
selection with authors of the previous commit. New commit changes
NoOp->FailTest+stopNode.

Please provide a PR to demonstrate your idea how to transfer and handle
exceptions. I believe it will not work because the fail handler is
activated from any pool inside a node.

ср, 5 дек. 2018 г. в 13:05, Anton Vinogradov <[hidden email]>:

> Dmitriy,
>
> >> Which code block will do a throw?
> Depends on the test.
>
> Looks like we make the *bad *test even *worse*.
>
> That's not a correct fix.
> In case you expect failure you have to check this expectation inside the
> special handler.
>
> I'd like to ask you to rollback these changes and replace them with correct
> fixes.
>
> On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <
> [hidden email]>
> wrote:
>
> > Hi,
> >
> > Dmitri,
> >
> > The meaningful failure handler as a default one looks reasonable.
> > Thanks a lot.
> >
> > But what is the reason to fallback to noop for 100+ test?
> > Does it means these test become failed after changing default failure
> > handler?
> > If so, let's create a ticket (may be umbrella) to investigate and fix
> this.
> >
> > I see 100+ touched files in PR and some of them are abstract classes, so,
> > we have much more affected tests.
> > Seems, most of failover test doesn't expects if any critical internal
> issue
> > occur and there is no need to fallback to noop.
> > Other test should set custom failure handler to detect expected failures
> or
> > if grid hanging simulation is needed (to keep hanged grid under control).
> >
> > On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <[hidden email]> wrote:
> >
> > > Dmitrii,
> > >
> > > No-op means "hide any problem", so, we lose the guarantees.
> > > Could you please share some examples where "no-op" better than "strict
> > > try-catch with a check"?
> > >
> > > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <[hidden email]>
> > > wrote:
> > >
> > > > Anton, I think wrapping every disconnecting node with try-catch will
> be
> > > > less readable than no-op handler.
> > > >
> > > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
> > > >
> > > > > Folks let me remind you that Dmitry changed default of ALL tests
> from
> > > > noop
> > > > > to a meaningful handler. So we should start every message here from
> > > > saying
> > > > > thank you to Dmitry.
> > > > >
> > > > > Please review remaining tests and remove noop where possible.
> > > > >
> > > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> > [hidden email]
> > > >:
> > > > >
> > > > > > Hi all,
> > > > > >
> > > > > > Really, why noop?
> > > > > >
> > > > > > If you expect failure handler should be triggered, you can
> override
> > > > > default
> > > > > > one and rise some flag, which can be checked in test.
> > > > > > This will make test clearer.
> > > > > >
> > > > > > With noop, you'll get previous unwanted behavior, that you are
> > > trying
> > > > to
> > > > > > improve, isnt'it?
> > > > > >
> > > > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <
> > [hidden email]>
> > > > > > написал:
> > > > > >
> > > > > > And you have to check the reason of failure inside the try-catch
> > > block,
> > > > > of
> > > > > > course.
> > > > > > In case found not equals to expected then test should rethrow the
> > > > > > exception.
> > > > > >
> > > > > >
> > > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:
> > > > > >
> > > > > >
> > > > > > > Dmitrii,
> > > > > > >
> > > > > > > The solution is not clear to me.
> > > > > > > In case you expect the failure then a correct case is to wrap
> it
> > > with
> > > > > > > try-catch block instead of no-op failure handler usage.
> > > > > > >
> > > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <
> > [hidden email]
> > > >:
> > > > > > >
> > > > > > >> Anton,
> > > > > > >>
> > > > > > >> Tests in these classes check fail cases when we expect
> critical
> > > > > > >> failure like node stop or exception thrown. Such tests trigger
> > > > failure
> > > > > > >> handler and it fails test when everything goes as it should
> go.
> > > > That's
> > > > > > >> why we need no-op handler here.
> > > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <
> [hidden email]
> > >:
> > > > > > >> >
> > > > > > >> > Hi Igniters,
> > > > > > >> >
> > > > > > >> > BTW, if you find in any of your tests it does't need an old
> > > value
> > > > of
> > > > > > >> > handler (=NoOp), feel free to remove it.
> > > > > > >> >
> > > > > > >> > Sincerely,
> > > > > > >> > Dmitriy Pavlov
> > > > > > >> >
> > > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <[hidden email]
> >:
> > > > > > >> >
> > > > > > >> > > Dmitrii,
> > > > > > >> > >
> > > > > > >> > > Could you please explain the reason of explicit set of
> 100+
> > > > > > >> > > NoOpFailureHandlers?
> > > > > > >> > >
> > > > > > >> > >
> > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > > > [hidden email]
> > > > > >:
> > > > > > >> > >
> > > > > > >> > > > Hello, Igniters!
> > > > > > >> > > >
> > > > > > >> > > > Today the test framework's default no-op failure handler
> > was
> > > > > > >> changed to
> > > > > > >> > > the
> > > > > > >> > > > handler, which stops the node and fails the test.
> > > > > > >> > > >
> > > > > > >> > > > Over 100 tests kept no-op failure handler by overrided
> > > > > > >> > > > `getFailureHandler()` method.
> > > > > > >> > > >
> > > > > > >> > > > If you'll found a problem or something unexpected -
> write
> > > here
> > > > > or
> > > > > > >> in the
> > > > > > >> > > > ticket [1].
> > > > > > >> > > >
> > > > > > >> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > > >> > > >
> > > > > > >> > >
> > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> >
> > --
> > Best regards,
> > Andrey V. Mashenkov
> >
>

Anton Vinogradov-2

Re: Default failure handler was changed for tests

Dmitriy,

As I said before, these changes allow tests to be successful in case of
unexpected failures.
That's not acceptable.

As a reviewer, you have to be ready to provide arguments why these tests
have to be fixed this way and what was the problem, in case you merged such
changes.
That's unacceptable to hide issues instead of fix.

Now, I ask you, as a reviewer, to provide the explanation.
What problem and at what test we solved by no-op handler.

And I'm going to rollback changes in case arguments will not be provided.

On Wed, Dec 5, 2018 at 1:10 PM Dmitriy Pavlov <[hidden email]> wrote:

> I will not do any rollback because changes make tests better. Please pay
> attention that no-op became default long time ago. Please discuss this
> selection with authors of the previous commit. New commit changes
> NoOp->FailTest+stopNode.
>
> Please provide a PR to demonstrate your idea how to transfer and handle
> exceptions. I believe it will not work because the fail handler is
> activated from any pool inside a node.
>
> ср, 5 дек. 2018 г. в 13:05, Anton Vinogradov <[hidden email]>:
>
> > Dmitriy,
> >
> > >> Which code block will do a throw?
> > Depends on the test.
> >
> > Looks like we make the *bad *test even *worse*.
> >
> > That's not a correct fix.
> > In case you expect failure you have to check this expectation inside the
> > special handler.
> >
> > I'd like to ask you to rollback these changes and replace them with
> correct
> > fixes.
> >
> > On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <
> > [hidden email]>
> > wrote:
> >
> > > Hi,
> > >
> > > Dmitri,
> > >
> > > The meaningful failure handler as a default one looks reasonable.
> > > Thanks a lot.
> > >
> > > But what is the reason to fallback to noop for 100+ test?
> > > Does it means these test become failed after changing default failure
> > > handler?
> > > If so, let's create a ticket (may be umbrella) to investigate and fix
> > this.
> > >
> > > I see 100+ touched files in PR and some of them are abstract classes,
> so,
> > > we have much more affected tests.
> > > Seems, most of failover test doesn't expects if any critical internal
> > issue
> > > occur and there is no need to fallback to noop.
> > > Other test should set custom failure handler to detect expected
> failures
> > or
> > > if grid hanging simulation is needed (to keep hanged grid under
> control).
> > >
> > > On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <[hidden email]>
> wrote:
> > >
> > > > Dmitrii,
> > > >
> > > > No-op means "hide any problem", so, we lose the guarantees.
> > > > Could you please share some examples where "no-op" better than
> "strict
> > > > try-catch with a check"?
> > > >
> > > > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <
> [hidden email]>
> > > > wrote:
> > > >
> > > > > Anton, I think wrapping every disconnecting node with try-catch
> will
> > be
> > > > > less readable than no-op handler.
> > > > >
> > > > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
> > > > >
> > > > > > Folks let me remind you that Dmitry changed default of ALL tests
> > from
> > > > > noop
> > > > > > to a meaningful handler. So we should start every message here
> from
> > > > > saying
> > > > > > thank you to Dmitry.
> > > > > >
> > > > > > Please review remaining tests and remove noop where possible.
> > > > > >
> > > > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> > > [hidden email]
> > > > >:
> > > > > >
> > > > > > > Hi all,
> > > > > > >
> > > > > > > Really, why noop?
> > > > > > >
> > > > > > > If you expect failure handler should be triggered, you can
> > override
> > > > > > default
> > > > > > > one and rise some flag, which can be checked in test.
> > > > > > > This will make test clearer.
> > > > > > >
> > > > > > > With noop, you'll get previous unwanted behavior, that you are
> > > > trying
> > > > > to
> > > > > > > improve, isnt'it?
> > > > > > >
> > > > > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <
> > > [hidden email]>
> > > > > > > написал:
> > > > > > >
> > > > > > > And you have to check the reason of failure inside the
> try-catch
> > > > block,
> > > > > > of
> > > > > > > course.
> > > > > > > In case found not equals to expected then test should rethrow
> the
> > > > > > > exception.
> > > > > > >
> > > > > > >
> > > > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]>:
> > > > > > >
> > > > > > >
> > > > > > > > Dmitrii,
> > > > > > > >
> > > > > > > > The solution is not clear to me.
> > > > > > > > In case you expect the failure then a correct case is to wrap
> > it
> > > > with
> > > > > > > > try-catch block instead of no-op failure handler usage.
> > > > > > > >
> > > > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <
> > > [hidden email]
> > > > >:
> > > > > > > >
> > > > > > > >> Anton,
> > > > > > > >>
> > > > > > > >> Tests in these classes check fail cases when we expect
> > critical
> > > > > > > >> failure like node stop or exception thrown. Such tests
> trigger
> > > > > failure
> > > > > > > >> handler and it fails test when everything goes as it should
> > go.
> > > > > That's
> > > > > > > >> why we need no-op handler here.
> > > > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <
> > [hidden email]
> > > >:
> > > > > > > >> >
> > > > > > > >> > Hi Igniters,
> > > > > > > >> >
> > > > > > > >> > BTW, if you find in any of your tests it does't need an
> old
> > > > value
> > > > > of
> > > > > > > >> > handler (=NoOp), feel free to remove it.
> > > > > > > >> >
> > > > > > > >> > Sincerely,
> > > > > > > >> > Dmitriy Pavlov
> > > > > > > >> >
> > > > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <
> [hidden email]
> > >:
> > > > > > > >> >
> > > > > > > >> > > Dmitrii,
> > > > > > > >> > >
> > > > > > > >> > > Could you please explain the reason of explicit set of
> > 100+
> > > > > > > >> > > NoOpFailureHandlers?
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > > > > [hidden email]
> > > > > > >:
> > > > > > > >> > >
> > > > > > > >> > > > Hello, Igniters!
> > > > > > > >> > > >
> > > > > > > >> > > > Today the test framework's default no-op failure
> handler
> > > was
> > > > > > > >> changed to
> > > > > > > >> > > the
> > > > > > > >> > > > handler, which stops the node and fails the test.
> > > > > > > >> > > >
> > > > > > > >> > > > Over 100 tests kept no-op failure handler by overrided
> > > > > > > >> > > > `getFailureHandler()` method.
> > > > > > > >> > > >
> > > > > > > >> > > > If you'll found a problem or something unexpected -
> > write
> > > > here
> > > > > > or
> > > > > > > >> in the
> > > > > > > >> > > > ticket [1].
> > > > > > > >> > > >
> > > > > > > >> > > > [1] https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > > > >> > > >
> > > > > > > >> > >
> > > > > > > >>
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > Best regards,
> > > Andrey V. Mashenkov
> > >
> >
>

Dmitry Pavlov

Re: Default failure handler was changed for tests

Anton, please provide PR to demo your idea. Code speaks louder than words
sometimes.

No reason to revert a contribution if someone has an idea, which is not
clear for others.

Again, we should discuss not Dmitrii contribution, but the initial
selection of no-op.

If you will do a test failure fixes later and you will set new handler
StopNode+FailTest as the only option - ok for me.

ср, 5 дек. 2018 г. в 13:35, Anton Vinogradov <[hidden email]>:

> Dmitriy,
>
> As I said before, these changes allow tests to be successful in case of
> unexpected failures.
> That's not acceptable.
>
> As a reviewer, you have to be ready to provide arguments why these tests
> have to be fixed this way and what was the problem, in case you merged such
> changes.
> That's unacceptable to hide issues instead of fix.
>
> Now, I ask you, as a reviewer, to provide the explanation.
> What problem and at what test we solved by no-op handler.
>
> And I'm going to rollback changes in case arguments will not be provided.
>
> On Wed, Dec 5, 2018 at 1:10 PM Dmitriy Pavlov <[hidden email]> wrote:
>
> > I will not do any rollback because changes make tests better. Please pay
> > attention that no-op became default long time ago. Please discuss this
> > selection with authors of the previous commit. New commit changes
> > NoOp->FailTest+stopNode.
> >
> > Please provide a PR to demonstrate your idea how to transfer and handle
> > exceptions. I believe it will not work because the fail handler is
> > activated from any pool inside a node.
> >
> > ср, 5 дек. 2018 г. в 13:05, Anton Vinogradov <[hidden email]>:
> >
> > > Dmitriy,
> > >
> > > >> Which code block will do a throw?
> > > Depends on the test.
> > >
> > > Looks like we make the *bad *test even *worse*.
> > >
> > > That's not a correct fix.
> > > In case you expect failure you have to check this expectation inside
> the
> > > special handler.
> > >
> > > I'd like to ask you to rollback these changes and replace them with
> > correct
> > > fixes.
> > >
> > > On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <
> > > [hidden email]>
> > > wrote:
> > >
> > > > Hi,
> > > >
> > > > Dmitri,
> > > >
> > > > The meaningful failure handler as a default one looks reasonable.
> > > > Thanks a lot.
> > > >
> > > > But what is the reason to fallback to noop for 100+ test?
> > > > Does it means these test become failed after changing default failure
> > > > handler?
> > > > If so, let's create a ticket (may be umbrella) to investigate and fix
> > > this.
> > > >
> > > > I see 100+ touched files in PR and some of them are abstract classes,
> > so,
> > > > we have much more affected tests.
> > > > Seems, most of failover test doesn't expects if any critical internal
> > > issue
> > > > occur and there is no need to fallback to noop.
> > > > Other test should set custom failure handler to detect expected
> > failures
> > > or
> > > > if grid hanging simulation is needed (to keep hanged grid under
> > control).
> > > >
> > > > On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <[hidden email]>
> > wrote:
> > > >
> > > > > Dmitrii,
> > > > >
> > > > > No-op means "hide any problem", so, we lose the guarantees.
> > > > > Could you please share some examples where "no-op" better than
> > "strict
> > > > > try-catch with a check"?
> > > > >
> > > > > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <
> > [hidden email]>
> > > > > wrote:
> > > > >
> > > > > > Anton, I think wrapping every disconnecting node with try-catch
> > will
> > > be
> > > > > > less readable than no-op handler.
> > > > > >
> > > > > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
> > > > > >
> > > > > > > Folks let me remind you that Dmitry changed default of ALL
> tests
> > > from
> > > > > > noop
> > > > > > > to a meaningful handler. So we should start every message here
> > from
> > > > > > saying
> > > > > > > thank you to Dmitry.
> > > > > > >
> > > > > > > Please review remaining tests and remove noop where possible.
> > > > > > >
> > > > > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> > > > [hidden email]
> > > > > >:
> > > > > > >
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > Really, why noop?
> > > > > > > >
> > > > > > > > If you expect failure handler should be triggered, you can
> > > override
> > > > > > > default
> > > > > > > > one and rise some flag, which can be checked in test.
> > > > > > > > This will make test clearer.
> > > > > > > >
> > > > > > > > With noop, you'll get previous unwanted behavior, that you
> are
> > > > > trying
> > > > > > to
> > > > > > > > improve, isnt'it?
> > > > > > > >
> > > > > > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <
> > > > [hidden email]>
> > > > > > > > написал:
> > > > > > > >
> > > > > > > > And you have to check the reason of failure inside the
> > try-catch
> > > > > block,
> > > > > > > of
> > > > > > > > course.
> > > > > > > > In case found not equals to expected then test should rethrow
> > the
> > > > > > > > exception.
> > > > > > > >
> > > > > > > >
> > > > > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <[hidden email]
> >:
> > > > > > > >
> > > > > > > >
> > > > > > > > > Dmitrii,
> > > > > > > > >
> > > > > > > > > The solution is not clear to me.
> > > > > > > > > In case you expect the failure then a correct case is to
> wrap
> > > it
> > > > > with
> > > > > > > > > try-catch block instead of no-op failure handler usage.
> > > > > > > > >
> > > > > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <
> > > > [hidden email]
> > > > > >:
> > > > > > > > >
> > > > > > > > >> Anton,
> > > > > > > > >>
> > > > > > > > >> Tests in these classes check fail cases when we expect
> > > critical
> > > > > > > > >> failure like node stop or exception thrown. Such tests
> > trigger
> > > > > > failure
> > > > > > > > >> handler and it fails test when everything goes as it
> should
> > > go.
> > > > > > That's
> > > > > > > > >> why we need no-op handler here.
> > > > > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <
> > > [hidden email]
> > > > >:
> > > > > > > > >> >
> > > > > > > > >> > Hi Igniters,
> > > > > > > > >> >
> > > > > > > > >> > BTW, if you find in any of your tests it does't need an
> > old
> > > > > value
> > > > > > of
> > > > > > > > >> > handler (=NoOp), feel free to remove it.
> > > > > > > > >> >
> > > > > > > > >> > Sincerely,
> > > > > > > > >> > Dmitriy Pavlov
> > > > > > > > >> >
> > > > > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <
> > [hidden email]
> > > >:
> > > > > > > > >> >
> > > > > > > > >> > > Dmitrii,
> > > > > > > > >> > >
> > > > > > > > >> > > Could you please explain the reason of explicit set of
> > > 100+
> > > > > > > > >> > > NoOpFailureHandlers?
> > > > > > > > >> > >
> > > > > > > > >> > >
> > > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > > > > > [hidden email]
> > > > > > > >:
> > > > > > > > >> > >
> > > > > > > > >> > > > Hello, Igniters!
> > > > > > > > >> > > >
> > > > > > > > >> > > > Today the test framework's default no-op failure
> > handler
> > > > was
> > > > > > > > >> changed to
> > > > > > > > >> > > the
> > > > > > > > >> > > > handler, which stops the node and fails the test.
> > > > > > > > >> > > >
> > > > > > > > >> > > > Over 100 tests kept no-op failure handler by
> overrided
> > > > > > > > >> > > > `getFailureHandler()` method.
> > > > > > > > >> > > >
> > > > > > > > >> > > > If you'll found a problem or something unexpected -
> > > write
> > > > > here
> > > > > > > or
> > > > > > > > >> in the
> > > > > > > > >> > > > ticket [1].
> > > > > > > > >> > > >
> > > > > > > > >> > > > [1]
> https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > > > > >> > > >
> > > > > > > > >> > >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > > Andrey V. Mashenkov
> > > >
> > >
> >
>

Anton Vinogradov-2

Re: Default failure handler was changed for tests

Dmitriy,

That's an incorrect idea to ask me to provide PR or to fix these test
properly since I'm not an author or reviewer.

But, I, as a community member, ask you to explain what problems the fix
fixes.
In case you're not able to provide the explanation I will rollback the
changes.

That's not acceptable to merge fix of unknown problems. At least, such "100
times copy-paste fix".
Please provide the explanation of the problem we're fixing for each test
group.

P.s. My goal is not to rollback something, but to prevent merge without
understanding what it fixes.

On Wed, Dec 5, 2018 at 1:40 PM Dmitriy Pavlov <[hidden email]> wrote:

> Anton, please provide PR to demo your idea. Code speaks louder than words
> sometimes.
>
> No reason to revert a contribution if someone has an idea, which is not
> clear for others.
>
> Again, we should discuss not Dmitrii contribution, but the initial
> selection of no-op.
>
> If you will do a test failure fixes later and you will set new handler
> StopNode+FailTest as the only option - ok for me.
>
> ср, 5 дек. 2018 г. в 13:35, Anton Vinogradov <[hidden email]>:
>
> > Dmitriy,
> >
> > As I said before, these changes allow tests to be successful in case of
> > unexpected failures.
> > That's not acceptable.
> >
> > As a reviewer, you have to be ready to provide arguments why these tests
> > have to be fixed this way and what was the problem, in case you merged
> such
> > changes.
> > That's unacceptable to hide issues instead of fix.
> >
> > Now, I ask you, as a reviewer, to provide the explanation.
> > What problem and at what test we solved by no-op handler.
> >
> > And I'm going to rollback changes in case arguments will not be provided.
> >
> > On Wed, Dec 5, 2018 at 1:10 PM Dmitriy Pavlov <[hidden email]>
> wrote:
> >
> > > I will not do any rollback because changes make tests better. Please
> pay
> > > attention that no-op became default long time ago. Please discuss this
> > > selection with authors of the previous commit. New commit changes
> > > NoOp->FailTest+stopNode.
> > >
> > > Please provide a PR to demonstrate your idea how to transfer and handle
> > > exceptions. I believe it will not work because the fail handler is
> > > activated from any pool inside a node.
> > >
> > > ср, 5 дек. 2018 г. в 13:05, Anton Vinogradov <[hidden email]>:
> > >
> > > > Dmitriy,
> > > >
> > > > >> Which code block will do a throw?
> > > > Depends on the test.
> > > >
> > > > Looks like we make the *bad *test even *worse*.
> > > >
> > > > That's not a correct fix.
> > > > In case you expect failure you have to check this expectation inside
> > the
> > > > special handler.
> > > >
> > > > I'd like to ask you to rollback these changes and replace them with
> > > correct
> > > > fixes.
> > > >
> > > > On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <
> > > > [hidden email]>
> > > > wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > Dmitri,
> > > > >
> > > > > The meaningful failure handler as a default one looks reasonable.
> > > > > Thanks a lot.
> > > > >
> > > > > But what is the reason to fallback to noop for 100+ test?
> > > > > Does it means these test become failed after changing default
> failure
> > > > > handler?
> > > > > If so, let's create a ticket (may be umbrella) to investigate and
> fix
> > > > this.
> > > > >
> > > > > I see 100+ touched files in PR and some of them are abstract
> classes,
> > > so,
> > > > > we have much more affected tests.
> > > > > Seems, most of failover test doesn't expects if any critical
> internal
> > > > issue
> > > > > occur and there is no need to fallback to noop.
> > > > > Other test should set custom failure handler to detect expected
> > > failures
> > > > or
> > > > > if grid hanging simulation is needed (to keep hanged grid under
> > > control).
> > > > >
> > > > > On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <[hidden email]>
> > > wrote:
> > > > >
> > > > > > Dmitrii,
> > > > > >
> > > > > > No-op means "hide any problem", so, we lose the guarantees.
> > > > > > Could you please share some examples where "no-op" better than
> > > "strict
> > > > > > try-catch with a check"?
> > > > > >
> > > > > > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <
> > > [hidden email]>
> > > > > > wrote:
> > > > > >
> > > > > > > Anton, I think wrapping every disconnecting node with try-catch
> > > will
> > > > be
> > > > > > > less readable than no-op handler.
> > > > > > >
> > > > > > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
> > > > > > >
> > > > > > > > Folks let me remind you that Dmitry changed default of ALL
> > tests
> > > > from
> > > > > > > noop
> > > > > > > > to a meaningful handler. So we should start every message
> here
> > > from
> > > > > > > saying
> > > > > > > > thank you to Dmitry.
> > > > > > > >
> > > > > > > > Please review remaining tests and remove noop where possible.
> > > > > > > >
> > > > > > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> > > > > [hidden email]
> > > > > > >:
> > > > > > > >
> > > > > > > > > Hi all,
> > > > > > > > >
> > > > > > > > > Really, why noop?
> > > > > > > > >
> > > > > > > > > If you expect failure handler should be triggered, you can
> > > > override
> > > > > > > > default
> > > > > > > > > one and rise some flag, which can be checked in test.
> > > > > > > > > This will make test clearer.
> > > > > > > > >
> > > > > > > > > With noop, you'll get previous unwanted behavior, that you
> > are
> > > > > > trying
> > > > > > > to
> > > > > > > > > improve, isnt'it?
> > > > > > > > >
> > > > > > > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <
> > > > > [hidden email]>
> > > > > > > > > написал:
> > > > > > > > >
> > > > > > > > > And you have to check the reason of failure inside the
> > > try-catch
> > > > > > block,
> > > > > > > > of
> > > > > > > > > course.
> > > > > > > > > In case found not equals to expected then test should
> rethrow
> > > the
> > > > > > > > > exception.
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <
> [hidden email]
> > >:
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > Dmitrii,
> > > > > > > > > >
> > > > > > > > > > The solution is not clear to me.
> > > > > > > > > > In case you expect the failure then a correct case is to
> > wrap
> > > > it
> > > > > > with
> > > > > > > > > > try-catch block instead of no-op failure handler usage.
> > > > > > > > > >
> > > > > > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <
> > > > > [hidden email]
> > > > > > >:
> > > > > > > > > >
> > > > > > > > > >> Anton,
> > > > > > > > > >>
> > > > > > > > > >> Tests in these classes check fail cases when we expect
> > > > critical
> > > > > > > > > >> failure like node stop or exception thrown. Such tests
> > > trigger
> > > > > > > failure
> > > > > > > > > >> handler and it fails test when everything goes as it
> > should
> > > > go.
> > > > > > > That's
> > > > > > > > > >> why we need no-op handler here.
> > > > > > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <
> > > > [hidden email]
> > > > > >:
> > > > > > > > > >> >
> > > > > > > > > >> > Hi Igniters,
> > > > > > > > > >> >
> > > > > > > > > >> > BTW, if you find in any of your tests it does't need
> an
> > > old
> > > > > > value
> > > > > > > of
> > > > > > > > > >> > handler (=NoOp), feel free to remove it.
> > > > > > > > > >> >
> > > > > > > > > >> > Sincerely,
> > > > > > > > > >> > Dmitriy Pavlov
> > > > > > > > > >> >
> > > > > > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <
> > > [hidden email]
> > > > >:
> > > > > > > > > >> >
> > > > > > > > > >> > > Dmitrii,
> > > > > > > > > >> > >
> > > > > > > > > >> > > Could you please explain the reason of explicit set
> of
> > > > 100+
> > > > > > > > > >> > > NoOpFailureHandlers?
> > > > > > > > > >> > >
> > > > > > > > > >> > >
> > > > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > > > > > > [hidden email]
> > > > > > > > >:
> > > > > > > > > >> > >
> > > > > > > > > >> > > > Hello, Igniters!
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > Today the test framework's default no-op failure
> > > handler
> > > > > was
> > > > > > > > > >> changed to
> > > > > > > > > >> > > the
> > > > > > > > > >> > > > handler, which stops the node and fails the test.
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > Over 100 tests kept no-op failure handler by
> > overrided
> > > > > > > > > >> > > > `getFailureHandler()` method.
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > If you'll found a problem or something unexpected
> -
> > > > write
> > > > > > here
> > > > > > > > or
> > > > > > > > > >> in the
> > > > > > > > > >> > > > ticket [1].
> > > > > > > > > >> > > >
> > > > > > > > > >> > > > [1]
> > https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > > > > > >> > > >
> > > > > > > > > >> > >
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best regards,
> > > > > Andrey V. Mashenkov
> > > > >
> > > >
> > >
> >
>

Ivan Pavlukhin

Re: Default failure handler was changed for tests

Hi Anton,

Could you please summarize what does aforementioned patch made really worse?

As I see, the patch added a very good thing -- meaningful failure
handler in tests. And I think it is really important. But was is the
harm and does it overweight positive result? And why?
ср, 5 дек. 2018 г. в 14:03, Anton Vinogradov <[hidden email]>:

>
> Dmitriy,
>
> That's an incorrect idea to ask me to provide PR or to fix these test
> properly since I'm not an author or reviewer.
>
> But, I, as a community member, ask you to explain what problems the fix
> fixes.
> In case you're not able to provide the explanation I will rollback the
> changes.
>
> That's not acceptable to merge fix of unknown problems. At least, such "100
> times copy-paste fix".
> Please provide the explanation of the problem we're fixing for each test
> group.
>
> P.s. My goal is not to rollback something, but to prevent merge without
> understanding what it fixes.
>
> On Wed, Dec 5, 2018 at 1:40 PM Dmitriy Pavlov <[hidden email]> wrote:
>
> > Anton, please provide PR to demo your idea. Code speaks louder than words
> > sometimes.
> >
> > No reason to revert a contribution if someone has an idea, which is not
> > clear for others.
> >
> > Again, we should discuss not Dmitrii contribution, but the initial
> > selection of no-op.
> >
> > If you will do a test failure fixes later and you will set new handler
> > StopNode+FailTest as the only option - ok for me.
> >
> > ср, 5 дек. 2018 г. в 13:35, Anton Vinogradov <[hidden email]>:
> >
> > > Dmitriy,
> > >
> > > As I said before, these changes allow tests to be successful in case of
> > > unexpected failures.
> > > That's not acceptable.
> > >
> > > As a reviewer, you have to be ready to provide arguments why these tests
> > > have to be fixed this way and what was the problem, in case you merged
> > such
> > > changes.
> > > That's unacceptable to hide issues instead of fix.
> > >
> > > Now, I ask you, as a reviewer, to provide the explanation.
> > > What problem and at what test we solved by no-op handler.
> > >
> > > And I'm going to rollback changes in case arguments will not be provided.
> > >
> > > On Wed, Dec 5, 2018 at 1:10 PM Dmitriy Pavlov <[hidden email]>
> > wrote:
> > >
> > > > I will not do any rollback because changes make tests better. Please
> > pay
> > > > attention that no-op became default long time ago. Please discuss this
> > > > selection with authors of the previous commit. New commit changes
> > > > NoOp->FailTest+stopNode.
> > > >
> > > > Please provide a PR to demonstrate your idea how to transfer and handle
> > > > exceptions. I believe it will not work because the fail handler is
> > > > activated from any pool inside a node.
> > > >
> > > > ср, 5 дек. 2018 г. в 13:05, Anton Vinogradov <[hidden email]>:
> > > >
> > > > > Dmitriy,
> > > > >
> > > > > >> Which code block will do a throw?
> > > > > Depends on the test.
> > > > >
> > > > > Looks like we make the *bad *test even *worse*.
> > > > >
> > > > > That's not a correct fix.
> > > > > In case you expect failure you have to check this expectation inside
> > > the
> > > > > special handler.
> > > > >
> > > > > I'd like to ask you to rollback these changes and replace them with
> > > > correct
> > > > > fixes.
> > > > >
> > > > > On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <
> > > > > [hidden email]>
> > > > > wrote:
> > > > >
> > > > > > Hi,
> > > > > >
> > > > > > Dmitri,
> > > > > >
> > > > > > The meaningful failure handler as a default one looks reasonable.
> > > > > > Thanks a lot.
> > > > > >
> > > > > > But what is the reason to fallback to noop for 100+ test?
> > > > > > Does it means these test become failed after changing default
> > failure
> > > > > > handler?
> > > > > > If so, let's create a ticket (may be umbrella) to investigate and
> > fix
> > > > > this.
> > > > > >
> > > > > > I see 100+ touched files in PR and some of them are abstract
> > classes,
> > > > so,
> > > > > > we have much more affected tests.
> > > > > > Seems, most of failover test doesn't expects if any critical
> > internal
> > > > > issue
> > > > > > occur and there is no need to fallback to noop.
> > > > > > Other test should set custom failure handler to detect expected
> > > > failures
> > > > > or
> > > > > > if grid hanging simulation is needed (to keep hanged grid under
> > > > control).
> > > > > >
> > > > > > On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <[hidden email]>
> > > > wrote:
> > > > > >
> > > > > > > Dmitrii,
> > > > > > >
> > > > > > > No-op means "hide any problem", so, we lose the guarantees.
> > > > > > > Could you please share some examples where "no-op" better than
> > > > "strict
> > > > > > > try-catch with a check"?
> > > > > > >
> > > > > > > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <
> > > > [hidden email]>
> > > > > > > wrote:
> > > > > > >
> > > > > > > > Anton, I think wrapping every disconnecting node with try-catch
> > > > will
> > > > > be
> > > > > > > > less readable than no-op handler.
> > > > > > > >
> > > > > > > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]:
> > > > > > > >
> > > > > > > > > Folks let me remind you that Dmitry changed default of ALL
> > > tests
> > > > > from
> > > > > > > > noop
> > > > > > > > > to a meaningful handler. So we should start every message
> > here
> > > > from
> > > > > > > > saying
> > > > > > > > > thank you to Dmitry.
> > > > > > > > >
> > > > > > > > > Please review remaining tests and remove noop where possible.
> > > > > > > > >
> > > > > > > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> > > > > > [hidden email]
> > > > > > > >:
> > > > > > > > >
> > > > > > > > > > Hi all,
> > > > > > > > > >
> > > > > > > > > > Really, why noop?
> > > > > > > > > >
> > > > > > > > > > If you expect failure handler should be triggered, you can
> > > > > override
> > > > > > > > > default
> > > > > > > > > > one and rise some flag, which can be checked in test.
> > > > > > > > > > This will make test clearer.
> > > > > > > > > >
> > > > > > > > > > With noop, you'll get previous unwanted behavior, that you
> > > are
> > > > > > > trying
> > > > > > > > to
> > > > > > > > > > improve, isnt'it?
> > > > > > > > > >
> > > > > > > > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <
> > > > > > [hidden email]>
> > > > > > > > > > написал:
> > > > > > > > > >
> > > > > > > > > > And you have to check the reason of failure inside the
> > > > try-catch
> > > > > > > block,
> > > > > > > > > of
> > > > > > > > > > course.
> > > > > > > > > > In case found not equals to expected then test should
> > rethrow
> > > > the
> > > > > > > > > > exception.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <
> > [hidden email]
> > > >:
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > Dmitrii,
> > > > > > > > > > >
> > > > > > > > > > > The solution is not clear to me.
> > > > > > > > > > > In case you expect the failure then a correct case is to
> > > wrap
> > > > > it
> > > > > > > with
> > > > > > > > > > > try-catch block instead of no-op failure handler usage.
> > > > > > > > > > >
> > > > > > > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <
> > > > > > [hidden email]
> > > > > > > >:
> > > > > > > > > > >
> > > > > > > > > > >> Anton,
> > > > > > > > > > >>
> > > > > > > > > > >> Tests in these classes check fail cases when we expect
> > > > > critical
> > > > > > > > > > >> failure like node stop or exception thrown. Such tests
> > > > trigger
> > > > > > > > failure
> > > > > > > > > > >> handler and it fails test when everything goes as it
> > > should
> > > > > go.
> > > > > > > > That's
> > > > > > > > > > >> why we need no-op handler here.
> > > > > > > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <
> > > > > [hidden email]
> > > > > > >:
> > > > > > > > > > >> >
> > > > > > > > > > >> > Hi Igniters,
> > > > > > > > > > >> >
> > > > > > > > > > >> > BTW, if you find in any of your tests it does't need
> > an
> > > > old
> > > > > > > value
> > > > > > > > of
> > > > > > > > > > >> > handler (=NoOp), feel free to remove it.
> > > > > > > > > > >> >
> > > > > > > > > > >> > Sincerely,
> > > > > > > > > > >> > Dmitriy Pavlov
> > > > > > > > > > >> >
> > > > > > > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <
> > > > [hidden email]
> > > > > >:
> > > > > > > > > > >> >
> > > > > > > > > > >> > > Dmitrii,
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > Could you please explain the reason of explicit set
> > of
> > > > > 100+
> > > > > > > > > > >> > > NoOpFailureHandlers?
> > > > > > > > > > >> > >
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > > > > > > > [hidden email]
> > > > > > > > > >:
> > > > > > > > > > >> > >
> > > > > > > > > > >> > > > Hello, Igniters!
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > > > Today the test framework's default no-op failure
> > > > handler
> > > > > > was
> > > > > > > > > > >> changed to
> > > > > > > > > > >> > > the
> > > > > > > > > > >> > > > handler, which stops the node and fails the test.
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > > > Over 100 tests kept no-op failure handler by
> > > overrided
> > > > > > > > > > >> > > > `getFailureHandler()` method.
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > > > If you'll found a problem or something unexpected
> > -
> > > > > write
> > > > > > > here
> > > > > > > > > or
> > > > > > > > > > >> in the
> > > > > > > > > > >> > > > ticket [1].
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > > > [1]
> > > https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > > > > > > >> > > >
> > > > > > > > > > >> > >
> > > > > > > > > > >>
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > > Andrey V. Mashenkov
> > > > > >
> > > > >
> > > >
> > >
> >

--
Best regards,
Ivan Pavlukhin

Anton Vinogradov-2

Re: Default failure handler was changed for tests

Ivan,

Do you mean massive no-op handler restore patch [1]?

[1] https://github.com/apache/ignite/pull/4974/files

On Wed, Dec 5, 2018 at 2:53 PM Павлухин Иван <[hidden email]> wrote:

> Hi Anton,
>
> Could you please summarize what does aforementioned patch made really
> worse?
>
> As I see, the patch added a very good thing -- meaningful failure
> handler in tests. And I think it is really important. But was is the
> harm and does it overweight positive result? And why?
> ср, 5 дек. 2018 г. в 14:03, Anton Vinogradov <[hidden email]>:
> >
> > Dmitriy,
> >
> > That's an incorrect idea to ask me to provide PR or to fix these test
> > properly since I'm not an author or reviewer.
> >
> > But, I, as a community member, ask you to explain what problems the fix
> > fixes.
> > In case you're not able to provide the explanation I will rollback the
> > changes.
> >
> > That's not acceptable to merge fix of unknown problems. At least, such
> "100
> > times copy-paste fix".
> > Please provide the explanation of the problem we're fixing for each test
> > group.
> >
> > P.s. My goal is not to rollback something, but to prevent merge without
> > understanding what it fixes.
> >
> > On Wed, Dec 5, 2018 at 1:40 PM Dmitriy Pavlov <[hidden email]>
> wrote:
> >
> > > Anton, please provide PR to demo your idea. Code speaks louder than
> words
> > > sometimes.
> > >
> > > No reason to revert a contribution if someone has an idea, which is not
> > > clear for others.
> > >
> > > Again, we should discuss not Dmitrii contribution, but the initial
> > > selection of no-op.
> > >
> > > If you will do a test failure fixes later and you will set new handler
> > > StopNode+FailTest as the only option - ok for me.
> > >
> > > ср, 5 дек. 2018 г. в 13:35, Anton Vinogradov <[hidden email]>:
> > >
> > > > Dmitriy,
> > > >
> > > > As I said before, these changes allow tests to be successful in case
> of
> > > > unexpected failures.
> > > > That's not acceptable.
> > > >
> > > > As a reviewer, you have to be ready to provide arguments why these
> tests
> > > > have to be fixed this way and what was the problem, in case you
> merged
> > > such
> > > > changes.
> > > > That's unacceptable to hide issues instead of fix.
> > > >
> > > > Now, I ask you, as a reviewer, to provide the explanation.
> > > > What problem and at what test we solved by no-op handler.
> > > >
> > > > And I'm going to rollback changes in case arguments will not be
> provided.
> > > >
> > > > On Wed, Dec 5, 2018 at 1:10 PM Dmitriy Pavlov <[hidden email]>
> > > wrote:
> > > >
> > > > > I will not do any rollback because changes make tests better.
> Please
> > > pay
> > > > > attention that no-op became default long time ago. Please discuss
> this
> > > > > selection with authors of the previous commit. New commit changes
> > > > > NoOp->FailTest+stopNode.
> > > > >
> > > > > Please provide a PR to demonstrate your idea how to transfer and
> handle
> > > > > exceptions. I believe it will not work because the fail handler is
> > > > > activated from any pool inside a node.
> > > > >
> > > > > ср, 5 дек. 2018 г. в 13:05, Anton Vinogradov <[hidden email]>:
> > > > >
> > > > > > Dmitriy,
> > > > > >
> > > > > > >> Which code block will do a throw?
> > > > > > Depends on the test.
> > > > > >
> > > > > > Looks like we make the *bad *test even *worse*.
> > > > > >
> > > > > > That's not a correct fix.
> > > > > > In case you expect failure you have to check this expectation
> inside
> > > > the
> > > > > > special handler.
> > > > > >
> > > > > > I'd like to ask you to rollback these changes and replace them
> with
> > > > > correct
> > > > > > fixes.
> > > > > >
> > > > > > On Wed, Dec 5, 2018 at 12:39 PM Andrey Mashenkov <
> > > > > > [hidden email]>
> > > > > > wrote:
> > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > Dmitri,
> > > > > > >
> > > > > > > The meaningful failure handler as a default one looks
> reasonable.
> > > > > > > Thanks a lot.
> > > > > > >
> > > > > > > But what is the reason to fallback to noop for 100+ test?
> > > > > > > Does it means these test become failed after changing default
> > > failure
> > > > > > > handler?
> > > > > > > If so, let's create a ticket (may be umbrella) to investigate
> and
> > > fix
> > > > > > this.
> > > > > > >
> > > > > > > I see 100+ touched files in PR and some of them are abstract
> > > classes,
> > > > > so,
> > > > > > > we have much more affected tests.
> > > > > > > Seems, most of failover test doesn't expects if any critical
> > > internal
> > > > > > issue
> > > > > > > occur and there is no need to fallback to noop.
> > > > > > > Other test should set custom failure handler to detect expected
> > > > > failures
> > > > > > or
> > > > > > > if grid hanging simulation is needed (to keep hanged grid under
> > > > > control).
> > > > > > >
> > > > > > > On Wed, Dec 5, 2018 at 12:16 PM Anton Vinogradov <
> [hidden email]>
> > > > > wrote:
> > > > > > >
> > > > > > > > Dmitrii,
> > > > > > > >
> > > > > > > > No-op means "hide any problem", so, we lose the guarantees.
> > > > > > > > Could you please share some examples where "no-op" better
> than
> > > > > "strict
> > > > > > > > try-catch with a check"?
> > > > > > > >
> > > > > > > > On Wed, Dec 5, 2018 at 11:37 AM Dmitrii Ryabov <
> > > > > [hidden email]>
> > > > > > > > wrote:
> > > > > > > >
> > > > > > > > > Anton, I think wrapping every disconnecting node with
> try-catch
> > > > > will
> > > > > > be
> > > > > > > > > less readable than no-op handler.
> > > > > > > > >
> > > > > > > > > ср, 5 дек. 2018 г., 9:26 Dmitriy Pavlov [hidden email]
> :
> > > > > > > > >
> > > > > > > > > > Folks let me remind you that Dmitry changed default of
> ALL
> > > > tests
> > > > > > from
> > > > > > > > > noop
> > > > > > > > > > to a meaningful handler. So we should start every message
> > > here
> > > > > from
> > > > > > > > > saying
> > > > > > > > > > thank you to Dmitry.
> > > > > > > > > >
> > > > > > > > > > Please review remaining tests and remove noop where
> possible.
> > > > > > > > > >
> > > > > > > > > > вт, 4 дек. 2018 г., 23:48 Andrey Mashenkov <
> > > > > > > [hidden email]
> > > > > > > > >:
> > > > > > > > > >
> > > > > > > > > > > Hi all,
> > > > > > > > > > >
> > > > > > > > > > > Really, why noop?
> > > > > > > > > > >
> > > > > > > > > > > If you expect failure handler should be triggered, you
> can
> > > > > > override
> > > > > > > > > > default
> > > > > > > > > > > one and rise some flag, which can be checked in test.
> > > > > > > > > > > This will make test clearer.
> > > > > > > > > > >
> > > > > > > > > > > With noop, you'll get previous unwanted behavior,
> that you
> > > > are
> > > > > > > > trying
> > > > > > > > > to
> > > > > > > > > > > improve, isnt'it?
> > > > > > > > > > >
> > > > > > > > > > > 4 дек. 2018 г. 23:25 пользователь "Anton Vinogradov" <
> > > > > > > [hidden email]>
> > > > > > > > > > > написал:
> > > > > > > > > > >
> > > > > > > > > > > And you have to check the reason of failure inside the
> > > > > try-catch
> > > > > > > > block,
> > > > > > > > > > of
> > > > > > > > > > > course.
> > > > > > > > > > > In case found not equals to expected then test should
> > > rethrow
> > > > > the
> > > > > > > > > > > exception.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > вт, 4 дек. 2018 г. в 23:21, Anton Vinogradov <
> > > [hidden email]
> > > > >:
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > Dmitrii,
> > > > > > > > > > > >
> > > > > > > > > > > > The solution is not clear to me.
> > > > > > > > > > > > In case you expect the failure then a correct case
> is to
> > > > wrap
> > > > > > it
> > > > > > > > with
> > > > > > > > > > > > try-catch block instead of no-op failure handler
> usage.
> > > > > > > > > > > >
> > > > > > > > > > > > вт, 4 дек. 2018 г. в 21:41, Dmitrii Ryabov <
> > > > > > > [hidden email]
> > > > > > > > >:
> > > > > > > > > > > >
> > > > > > > > > > > >> Anton,
> > > > > > > > > > > >>
> > > > > > > > > > > >> Tests in these classes check fail cases when we
> expect
> > > > > > critical
> > > > > > > > > > > >> failure like node stop or exception thrown. Such
> tests
> > > > > trigger
> > > > > > > > > failure
> > > > > > > > > > > >> handler and it fails test when everything goes as it
> > > > should
> > > > > > go.
> > > > > > > > > That's
> > > > > > > > > > > >> why we need no-op handler here.
> > > > > > > > > > > >> вт, 4 дек. 2018 г. в 20:06, Dmitriy Pavlov <
> > > > > > [hidden email]
> > > > > > > >:
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > Hi Igniters,
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > BTW, if you find in any of your tests it does't
> need
> > > an
> > > > > old
> > > > > > > > value
> > > > > > > > > of
> > > > > > > > > > > >> > handler (=NoOp), feel free to remove it.
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > Sincerely,
> > > > > > > > > > > >> > Dmitriy Pavlov
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > вт, 4 дек. 2018 г. в 20:02, Anton Vinogradov <
> > > > > [hidden email]
> > > > > > >:
> > > > > > > > > > > >> >
> > > > > > > > > > > >> > > Dmitrii,
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > > Could you please explain the reason of explicit
> set
> > > of
> > > > > > 100+
> > > > > > > > > > > >> > > NoOpFailureHandlers?
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii Ryabov <
> > > > > > > > > [hidden email]
> > > > > > > > > > >:
> > > > > > > > > > > >> > >
> > > > > > > > > > > >> > > > Hello, Igniters!
> > > > > > > > > > > >> > > >
> > > > > > > > > > > >> > > > Today the test framework's default no-op
> failure
> > > > > handler
> > > > > > > was
> > > > > > > > > > > >> changed to
> > > > > > > > > > > >> > > the
> > > > > > > > > > > >> > > > handler, which stops the node and fails the
> test.
> > > > > > > > > > > >> > > >
> > > > > > > > > > > >> > > > Over 100 tests kept no-op failure handler by
> > > > overrided
> > > > > > > > > > > >> > > > `getFailureHandler()` method.
> > > > > > > > > > > >> > > >
> > > > > > > > > > > >> > > > If you'll found a problem or something
> unexpected
> > > -
> > > > > > write
> > > > > > > > here
> > > > > > > > > > or
> > > > > > > > > > > >> in the
> > > > > > > > > > > >> > > > ticket [1].
> > > > > > > > > > > >> > > >
> > > > > > > > > > > >> > > > [1]
> > > > https://issues.apache.org/jira/browse/IGNITE-8227
> > > > > > > > > > > >> > > >
> > > > > > > > > > > >> > >
> > > > > > > > > > > >>
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Best regards,
> > > > > > > Andrey V. Mashenkov
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
>
>
>
> --
> Best regards,
> Ivan Pavlukhin
>

Dmitry Pavlov

Re: Default failure handler was changed for tests

In reply to this post by Ivan Pavlukhin

Dmitrii Ryabov explained these tests are perfectly ok to have failures as
these tests do test failures.

Anton, there is no reason to revert other's contributions because you know
how to do things better. A lot of people can do things better than me.
Should we revert everything I've contributed? I hope - no.

If you can do things better, just commit further improvements. And I will
be happy if you contribute some improvements later.

If you would like to revert by veto, please justify your intent. If you
would discuss it with all community, please feel free to convince me and
others.

ср, 5 дек. 2018 г. в 14:53, Павлухин Иван <[hidden email]>:

1234