Anton, I disagree with this approach: "You will ask, other will provide explanations/excuses/apology and so on". Since you rejecting to chime in and help this means trying to manage instead of doing.
I don't ask you to re-do this change, I ask to demonstrate any better approach for tests which intentionally activate failure handler. I think the only option is NoOp - we see this approach here. Just take 1 test and prove your concept using any other option. If we don't have other options, there is nothing to discuss why it is NoOp for 100 tests. ср, 5 дек. 2018 г. в 15:38, Anton Vinogradov <a...@apache.org>: > 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 Павлухин Иван <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 > > >
