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 <dpav...@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 <a...@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.shangar...@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 Павлухин Иван <vololo...@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 <dpav...@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, Павлухин Иван <vololo...@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 <a...@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 < > > dpav...@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 <a...@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 < > > > > dpav...@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 < > > a...@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.mashen...@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 < > > > > > > a...@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 < > > > > > > > > > > somefire...@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 > > > > dpav...@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.mashen...@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" < > > > > > > > > > > > > a...@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 > < > > > > > > > > a...@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 > < > > > > > > > > > > > > somefire...@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 < > > > > > > > > > > > dpav...@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 < > > > > > > > > > > a...@apache.org > > > > > > > > > > > >: > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > >> > > Dmitrii, > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > Could you please explain the reason of > > > > explicit > > > > > > set > > > > > > > > of > > > > > > > > > > > 100+ > > > > > > > > > > > > > > > > >> > > NoOpFailureHandlers? > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > >> > > вт, 4 дек. 2018 г. в 19:12, Dmitrii > > > Ryabov < > > > > > > > > > > > > > > somefire...@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 > > > > > > > > > >
