ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Pavlov <dpav...@apache.org>
Subject Re: Default failure handler was changed for tests
Date Wed, 05 Dec 2018 10:10:46 GMT
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