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 <a...@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 Павлухин Иван <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 >> >
