ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Anton Vinogradov ...@apache.org>
Subject Re: Default failure handler was changed for tests
Date Wed, 05 Dec 2018 10:35:31 GMT
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 <dpavlov@apache.org> 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 <av@apache.org>:
>
> > 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 <
> > andrey.mashenkov@gmail.com>
> > 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 <av@apache.org>
> 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 <
> somefireone@gmail.com>
> > > > 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 dpavlov@apache.org:
> > > > >
> > > > > > 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 <
> > > andrey.mashenkov@gmail.com
> > > > >:
> > > > > >
> > > > > > > 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" <
> > > av@apache.org>
> > > > > > > написал:
> > > > > > >
> > > > > > > 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 <av@apache.org>:
> > > > > > >
> > > > > > >
> > > > > > > > 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
<
> > > somefireone@gmail.com
> > > > >:
> > > > > > > >
> > > > > > > >> 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
<
> > dpavlov@apache.org
> > > >:
> > > > > > > >> >
> > > > > > > >> > 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 <
> av@apache.org
> > >:
> > > > > > > >> >
> > > > > > > >> > > Dmitrii,
> > > > > > > >> > >
> > > > > > > >> > > Could you please explain the reason
of explicit set of
> > 100+
> > > > > > > >> > > NoOpFailureHandlers?
> > > > > > > >> > >
> > > > > > > >> > >
> > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii
Ryabov <
> > > > > somefireone@gmail.com
> > > > > > >:
> > > > > > > >> > >
> > > > > > > >> > > > 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
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message