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