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 14:27:57 GMT
Andrey,

>> But why should we make all things perfect
>> in a single fix?
As I said, I'm ok in case someone ready to continue :)
But, we should avoid such over-copy-pasted commits in the future.

On Wed, Dec 5, 2018 at 5:13 PM Andrey Mashenkov <andrey.mashenkov@gmail.com>
wrote:

> Dmitry,
>
> Do we have TC run results for the PR before massive failure handler
> fallbacks were added?
> Let's create a ticket to investigate possibility of using any meaningful
> failure handler for such tests with TC report attached.
>
> On Wed, Dec 5, 2018 at 4:41 PM Anton Vinogradov <av@apache.org> wrote:
>
> > Dmitriy,
> >
> > It's ok in case someone ready to do this (get rid of all no-op or explain
> > why it's a better choice).
> > Explicit confirmation required.
> >
> > Otherwise, only rollback is an option.
> >
> > On Wed, Dec 5, 2018 at 4:29 PM Dmitriy Pavlov <dpavlov@apache.org>
> wrote:
> >
> > > Anton, if you care enough here will you try to research a couple of
> these
> > > tests? Or you are asking others to do things for you, aren't you?
> > >
> > > I like idea from Andrew to create ticket and check these test to keep
> > > moving towards 0....10 tests with noop. It is easy to locate these
> > > overridden method now.
> > >
> > > So threat this change as contributed mechanism for failing tests. Is it
> > Ok
> > > for you?
> > >
> > > ср, 5 дек. 2018 г., 15:59 Anton Vinogradov <av@apache.org>:
> > >
> > > > >> I didn't get. What is the problem in saving No-Op for several
> tests?
> > > Why
> > > > >> should we keep No-Op for all?
> > > > Several (less than 10) is ok to me with the proper explanation why
> > tests
> > > > fail and why no-op is a better choice.
> > > >
> > > > 100+++ copy-pasted no-op handlers are not ok!
> > > >
> > > > >> I don't ask you to re-do this change, I ask to demonstrate any
> > better
> > > > >> approach for tests which intentionally activate failure handler.
> > > > You asking me to provide approach without explanation why tests fail
> > > > without no-op handler?
> > > > My approach is to rollback this fix, reopen the issue and make
> > everything
> > > > properly.
> > > > Make a proper investigation first.
> > > >
> > > >
> > > > Finally, let's stop this game.
> > > > We have to discuss the reasons why tests fail.
> > > > In case no-one checked "why" before the fix was merged we will be
> able
> > to
> > > > start doing this after rollback.
> > > >
> > > > On Wed, Dec 5, 2018 at 3:49 PM Eduard Shangareev <
> > > > eduard.shangareev@gmail.com> wrote:
> > > >
> > > > > 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
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
> --
> Best regards,
> Andrey V. Mashenkov
>

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