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 >
