ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Eduard Shangareev <eduard.shangar...@gmail.com>
Subject Re: Default failure handler was changed for tests
Date Wed, 05 Dec 2018 12:38:26 GMT
Guys,

I didn't get. What is the problem in saving No-Op for several tests? Why
should we keep No-Op for all?

On Wed, Dec 5, 2018 at 3:20 PM Павлухин Иван <vololo100@gmail.com> wrote:

> Anton,
>
> Yes I meant that patch. And I would like to respell a name "massive
> no-op handler restore" to "use no-op failure handler only where it is
> assumed".
> ср, 5 дек. 2018 г. в 15:09, Dmitriy Pavlov <dpavlov@apache.org>:
> >
> > 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, Павлухин Иван <vololo100@gmail.com>:
> >
> > > 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 <av@apache.org>:
> > > >
> > > > 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 <dpavlov@apache.org>
> > > 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 <av@apache.org>:
> > > > >
> > > > > > 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
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > > Ivan Pavlukhin
> > >
>
>
>
> --
> Best regards,
> Ivan Pavlukhin
>

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