Guys, I asked what harm will applying the patch bring I have not got a direct answer. But I think I got some pain points: 1. Anton does not like that reasons why ~100 tests require noop handler are not clear. And might be several problems are covered there. 2. Nikolay suggests some code improvements.
Nikolay's patch [1] suggests a slightly different approach how to the same thing. And implementing that idea looks like a cheap refactoring. But the idea of course could be discussed. Straight away I can suggest another slightly different trick [2]. Investigating why ~100 tests require noop handler could be costly. So, in that direction I see following options which can happen for sure: 1. Accept the patch and bring an improvement to Ignite (and create a ticket for further investigation). 2. Revert the patch and loose an improvement. One might say that there is an option "Revert the patch and then do it better" but I does not see anything (anyone) what can guarantee it. So, I personally prefer an option 1 against 2 because I believe that it is good if the system "can make a progress". [1] https://github.com/apache/ignite/pull/5584/files [2] https://github.com/apache/ignite/pull/5586/files ср, 5 дек. 2018 г. в 21:22, Nikolay Izhikov <nizhi...@apache.org>: > > Dmitriy. > > > The closest analog to Noop handler is mute of test failure. > > By this commit, we had unmuted (possible) failures in ~50000-~100=~49900 > tests, and we’re still concerned about style or minor details if no-op was > copy-pasted, aren’t we? > > Can you explain this idea a bit more? > I don't understand what is unmuted by discussed commit. > > ср, 5 дек. 2018 г. в 20:40, Nikolay Izhikov <nizhi...@apache.org>: > > > > Thanks, as an improvement to the code, this may be better. > > > > I can prepare a full patch for NoOp handler. > > What do you think? > > > > Anton Vinogradov, do you agree with this approach? > > > > > > > > ср, 5 дек. 2018 г. в 20:33, Dmitriy Pavlov <dpav...@apache.org>: > > > >> Thanks, as an improvement to the code, this may be better. But still, it > >> is > >> not a reason to revert. And Anton mentioned something with better > >> exception > >> handling/logging. Probably we will see an implementation as well. > >> > >> This case here is a big thing related to The Apache Way, - and I'll > >> explain > >> why it makes me switched into fight-mode - until we stop this nonsense. If > >> PMCs (at least) are aware of patterns and anti-patterns in the community, > >> we will succeed as a project much more as with (only) perfect code. > >> > >> The closest analog to Noop handler is mute of test failure. By this > >> commit, > >> we had unmuted (possible) failures in ~50000-~100=~49900 tests, and we’re > >> still concerned about style or minor details if no-op was copy-pasted, > >> aren’t we? > >> > >> To everyone arguing about the number of tests we are allowed to have with > >> no-op: please visit this page > >> > >> https://ci.ignite.apache.org/project.html?projectId=IgniteTests24Java8&tab=mutedProblems&branch_IgniteTests24Java8=__all_branches__ > >> > >> It says: Muted tests: 3154. Are there any disagreements here? Why there > >> are > >> no insistent disagreement/not happy PMCs with absolutely unconditionally > >> muted failures? > >> > >> Any reason now to continue the discussion about reverting absolutely > >> positive contribution into product stability from Dmitrii R.? > >> > >> Moreover, Dmitrii Ryabov is trying to solve odd mutes problem, as well, to > >> locate mutes with links resolved issues in the TC Bot. Is he deserved to > >> read denouncing comments about the contribution? I guess, no, especially > >> if > >> the commenter is not going to help/contribute a better fix. > >> > >> This is now a paramount thing for me if people in this thread will join > >> the > >> process or not. People may be not happy with some decisions/code/style, > >> and > >> some people are more often unhappy than others. More you contribute,- more > >> you can decide. If you don't contribute at all - I don't care too much > >> about just opinions, I can accept facts. To provide facts we need to do > >> deep research, how can someone know if the test should be no-op or not > >> without deep analysis? > >> > >> Again, if someone comes to list and provide just negative feedback, people > >> will stop writing here. Probably no-op was enabled without proper > >> discussion because of this, someone may be afraid of sharing this. Result: > >> some of us knew it only now. > >> > >> Do you need to make Ignite quite toxic place to have an absolutely perfect > >> code with just a few of arguing-resistant contributors? I believe not, and > >> you don't need to be reminded 'community first principle'. > >> > >> > >> ср, 5 дек. 2018 г. в 19:43, Nikolay Izhikov <nizhi...@apache.org>: > >> > >> > Dmitriy. > >> > > >> > I think we should avoid copy paste code instead of thinking about Apache > >> > Way all the time :) > >> > > >> > Anyway, I propose to return to the code! > >> > I think we should use some kind of marker base class for a cases with > >> > NoOpHandler. > >> > This has several advantages, comparing with current implementation: > >> > > >> > 1. No copy paste code > >> > 2. Reduce changes. > >> > 3. All usages of NoOpHandler can be easily found with IDE or grep > >> search. > >> > > >> > I've prepared proof of concept pull request to demonstrate my approach > >> [1] > >> > I can go further and prepare full fix. > >> > > >> > What do you think? > >> > > >> > [1] https://github.com/apache/ignite/pull/5584/files > >> > > >> > ср, 5 дек. 2018 г. в 18:29, Dmitriy Pavlov <dpav...@apache.org>: > >> > > >> > > Folks, let me explain one thing which is not related much to fix > >> itself, > >> > > but it is more about how we interact. If someone will just come to the > >> > list > >> > > and say it is not good commit, it is a silly solution and say to > >> others > >> > to > >> > > rework these patches - it is a road to nowhere. > >> > > > >> > > If someone sees the potential to make things better he or she suggest > >> > help > >> > > (or commits patch). This is named do-ocracy, those who do can make a > >> > > decision. > >> > > > >> > > > >> > > > >> > > And this topic it is a perfect example of how do-ocracy should (and > >> > should > >> > > not) work. We have a potentially hidden problem (we had it before > >> Dmitriy > >> > > R. commit), I believe 3 or 7 tests may be found after re-checks of > >> tests. > >> > > Eventually, these tests will get their stop-node handler after > >> revisiting > >> > > no-op test list. > >> > > > >> > > > >> > > > >> > > We have ~100 tests and several people who care. Anton, Andrew, > >> Dmitrii & > >> > > Dmitriy, Nikolay, probably Ed, and we have 100/6 = 18 tests to double > >> > check > >> > > for each contributor. We can make things better if we go together. And > >> > this > >> > > is how a community works. > >> > > > >> > > > >> > > > >> > > If someone just come to list to criticize and enforces someone else > >> to do > >> > > all things, he or she probably don't want to improve project code but > >> has > >> > > other goals. > >> > > > >> > > ср, 5 дек. 2018 г. в 18:08, Andrey Kuznetsov <stku...@gmail.com>: > >> > > > >> > > > As I can see from the above discussion, > >> > > > > >> > > > > Tests in these classes check fail cases when we expect critical > >> > > failure > >> > > > like node stop or exception thrown > >> > > > > >> > > > So, this copy-n-paste-style change is caused by the imperfect logic > >> of > >> > > > existing tests, that should be reworked in more robust way, e.g. > >> using > >> > > > custom failure handlers. Dmitrii just revealed the existing flaws, > >> IMO. > >> > > > > >> > > > ср, 5 дек. 2018 г. в 17:54, Nikolay Izhikov <nizhi...@apache.org>: > >> > > > > >> > > > > Hello, Igniters. > >> > > > > > >> > > > > I'm agree with Anton Vinogradov. > >> > > > > > >> > > > > I think we should avoid commits like [1] > >> > > > > Copy paste coding style is well known anti pattern. > >> > > > > > >> > > > > Don't we have another option to do same fix with better styling? > >> > > > > > >> > > > > Accepting such patches leads to the further tickets to cleanup > >> mess > >> > > that > >> > > > > patches brings to the code base. > >> > > > > Example of cleanup [2] > >> > > > > > >> > > > > It's take a significant amount of my and Maxim time to made and > >> > review > >> > > > this > >> > > > > cleanup patch. > >> > > > > > >> > > > > We shouldn't accept patch with copy paste "improvements". > >> > > > > > >> > > > > > I really like your perfectionism > >> > > > > > >> > > > > It's not about perfectionism it's about keeping code base clean. > >> > > > > > >> > > > > > And I'm going to rollback changes in case arguments will not be > >> > > > provided. > >> > > > > > >> > > > > +1 to rollback and rework this commit. > >> > > > > At least, we should reduce copy paste code. > >> > > > > > >> > > > > [1] > >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> https://github.com/apache/ignite/commit/b94a3c2fe3a272a31fad62b80505d16f87eab2dd > >> > > > > [2] > >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> https://github.com/apache/ignite/commit/eb8038f65285559c5424eba2882b0de0583ea7af > >> > > > > > >> > > > > ср, 5 дек. 2018 г. в 17:28, Anton Vinogradov <a...@apache.org>: > >> > > > > > >> > > > > > Andrey, > >> > > > > > > >> > > > > > >> But why should we make all things perfect > >> > > > > > >> in a single fix? > >> > > > > > As I said, I'm ok in case someone ready to continue :) > >> > > > > > But, we should avoid such over-copy-pasted commits in the > >> future. > >> > > > > > > >> > > > > > On Wed, Dec 5, 2018 at 5:13 PM Andrey Mashenkov < > >> > > > > > andrey.mashen...@gmail.com> > >> > > > > > wrote: > >> > > > > > > >> > > > > > > Dmitry, > >> > > > > > > > >> > > > > > > Do we have TC run results for the PR before massive failure > >> > handler > >> > > > > > > fallbacks were added? > >> > > > > > > Let's create a ticket to investigate possibility of using any > >> > > > > meaningful > >> > > > > > > failure handler for such tests with TC report attached. > >> > > > > > > > >> > > > > > > On Wed, Dec 5, 2018 at 4:41 PM Anton Vinogradov < > >> a...@apache.org> > >> > > > wrote: > >> > > > > > > > >> > > > > > > > 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 > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > -- > >> > > > > > > Best regards, > >> > > > > > > Andrey V. Mashenkov > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > >> > > > -- > >> > > > Best regards, > >> > > > Andrey Kuznetsov. > >> > > > > >> > > > >> > > >> > > -- Best regards, Ivan Pavlukhin
