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:42:45 GMT
Ivan,

>> 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".

How about "we changed some handlers to proper, but keep other no-ops using
explicit copy-paste"? :)

On Wed, Dec 5, 2018 at 3:38 PM Anton Vinogradov <av@apache.org> wrote:

> 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