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 12:08:51 GMT
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
>

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