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 12:38:30 GMT
Dmitriy Pavlov, Dmitrii Ryabov,

>> Anton, there is no reason to revert other's contributions because you
know
>> how to do things better.

What I see is "We replaced no-op with the proper handler, but ..... 100+
no-op still here because tests start failing :)"
That's a completely different situation.
And it's unacceptable to merge not a finished solution.

A proper explanation of problems why these tests have to have no-op handler
still required.

Once proper explanation will be provided we will be able to decide is it ok
or no.
Explanation lack means commit rollback and issue reopening and there is
nothing to discuss.

Please provide at least some examples with the following template
Test XXX required no-op handler because of YYY
and we will agree or will try to find a better solution.

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