Hi all, I think, I've done with this issue, what should we do next?
PR: https://github.com/apache/ignite/pull/3542 Upsource: https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502 TC: https://ci.ignite.apache.org/viewModification.html?modId=723895&personal=false&buildTypeId=&tab=vcsModificationTests JIRA: https://issues.apache.org/jira/browse/IGNITE-6842 чт, 22 февр. 2018 г. в 14:12, Dmitry Pavlov <dpavlov....@gmail.com>: > Hi Maxim, > > Thank you. > > To my mind stopAllGrids call should be kept in afterTestsStop(). If > developer forgot to call super(), he will see exception from > beforeTestsStart() > added by you. > > If you think patch is ready to be reviewed, please change JIRA status to > "Patch Available". > > Optionally you can create Upsource review. It would be easier for multiple > reviewers to handle this patch. This test framework is used by all Igniters > so Upsource would be good option (https://reviews.ignite.apache.org/ ). > > Sincerely, > Dmitriy Pavlov > > чт, 22 февр. 2018 г. в 13:44, Maxim Muzafarov <maxmu...@gmail.com>: > > > Hi all, > > > > I've made some changes based on our previous discusstions, please see PR > > [1]: > > 1) Remove duplicated code for stopGrid (by index and by name); > > 2) Add new method that thows exception if not all grids haven't stopped > > correctly; > > 3) Change tests that have been affected by this changes; > > > > Also, I have some thoughts for clarification: > > 1) beforeTestsStart() - I expect here in subtests that grids are not > > started yet. Am I right? > > 2) I think we should call stopAllGrids in tearDown method. So, if in some > > cases we'll override afterTestsStop in subclasses and forgot to call > > super() it won't lead us to exception. > > > > [1] https://github.com/apache/ignite/pull/3542 > > [2] https://ci.ignite.apache.org/viewModification.html?modId=717275 > > [3] https://issues.apache.org/jira/browse/IGNITE-6842 > > > > > > ср, 7 февр. 2018 г. в 18:28, Maxim Muzafarov <maxmu...@gmail.com>: > > > > > Thank you all, > > > > > > I'll add this comment's for JIRA ticket, if you don't mind. > > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <dpavlov....@gmail.com>: > > > > > >> Yes, this solution allows to cover both cases: > > >> a) not stopped node from previous test and > > >> b) allows to remove useless code that stops Ignite nodes from each > test. > > >> > > >> ср, 7 февр. 2018 г. в 15:13, Anton Vinogradov < > avinogra...@gridgain.com > > >: > > >> > > >> > Maxim, > > >> > > > >> > We discussed with Dima privately, and decided > > >> > > > >> > 1) We have to assert that there is no alive nodes at > > GridAbstractTest's > > >> > beforeTestsStarted > > >> > 2) We have to kill all alive nodes (without force) at > > GridAbstractTest's > > >> > afterTestsStopped > > >> > 3) In case of any exceptions at #2 we have to see test error > > >> > 4) We can get rid of all useless stopAllGrids at GridAbstractTest's > > >> > subclasses. > > >> > > > >> > > > >> > > > >> > On Wed, Feb 7, 2018 at 2:32 PM, Dmitry Pavlov < > dpavlov....@gmail.com> > > >> > wrote: > > >> > > > >> > > > Let's just add stopAllGrids(flase) to GridAbstractTest 's > > >> > > afterTestsStopped > > >> > > method body. > > >> > > > > >> > > Can't agree with it becase implicit silent shutdown of nodes from > > test > > >> > > framework may cause a lot of bugs introduced to Ignite. > > >> > > > > >> > > I think stop+test fail should occur. > > >> > > In that case author of incorrect test or not consistent Ignite > > >> shutdown > > >> > > will see problem. > > >> > > > > >> > > 'Fail fast' if always better than hidding real problem under > > automatic > > >> > > framework feature. > > >> > > > > >> > > ср, 7 февр. 2018 г. в 14:05, Anton Vinogradov < > > >> avinogra...@gridgain.com > > >> > >: > > >> > > > > >> > > > Hi all, > > >> > > > > > >> > > > > I've made some research about this problem and i think that in > > >> > general > > >> > > we > > >> > > > > should move stopAllGrids method in GridAbstractTest class to > > >> > > > > afterTestsStopped method with some changes. Am I right? > > >> > > > Let's just add stopAllGrids(flase) to GridAbstractTest 's > > >> > > > afterTestsStopped method > > >> > > > body. > > >> > > > > > >> > > > > I have a question about stopAllGrids(boolean cancel) this > > "cancel" > > >> > > > That's just a flag means "do not wait for any operations finish" > > >> > > > See no reason to set it true in that case. > > >> > > > > > >> > > > > If you delegate closing to afterTestsStopped this will affect > > only > > >> > > > > last test (method). > > >> > > > The idea is to stop all nodes at last test's method finish. > > >> > > > > > >> > > > > Nodes that survive between tests can affect successive > > >> > > > tests. > > >> > > > Thats ok. we have a lot tests where nodes reusable between > test's > > >> > > methods. > > >> > > > > > >> > > > > > >> > > > On Wed, Feb 7, 2018 at 1:20 PM, Dmitry Pavlov < > > >> dpavlov....@gmail.com> > > >> > > > wrote: > > >> > > > > > >> > > > > Hi Igniters, > > >> > > > > > > >> > > > > IMO nodes that survive between tests is not general practice > and > > >> I'm > > >> > > not > > >> > > > > sure is a best practice. I suggest to mark such tests with > some > > >> > method > > >> > > > > overriden from AbstractTest. > > >> > > > > > > >> > > > > About cancel flag, please note stopAllGrids(boolean cancel) > > >> > > cancel=false > > >> > > > > allows to wait of checkpoint ends in case persistence enabled. > > >> > > > > > > >> > > > > I still suggest to avoid stopping any nodes by test, but > > validate > > >> not > > >> > > > > stopped node exist and fail test instead of siltent implicit > > >> actions. > > >> > > > > > > >> > > > > Sincerely, > > >> > > > > Dmitriy Pavlov > > >> > > > > > > >> > > > > ср, 7 февр. 2018 г. в 13:04, Andrey Kuznetsov < > > stku...@gmail.com > > >> >: > > >> > > > > > > >> > > > > > Hi Maxim, > > >> > > > > > > > >> > > > > > Regarding your first question, the use of afterTestsStopped > is > > >> not > > >> > > > enough > > >> > > > > > to stop all nodes, since each individual test (method) can > > start > > >> > > custom > > >> > > > > set > > >> > > > > > of notes during its operation, and this very test should > stop > > >> all > > >> > > those > > >> > > > > > nodes. If you delegate closing to afterTestsStopped this > will > > >> > affect > > >> > > > only > > >> > > > > > last test (method). Nodes that survive between tests can > > affect > > >> > > > > successive > > >> > > > > > tests. > > >> > > > > > > > >> > > > > > 2018-02-07 1:10 GMT+03:00 Maxim Muzafarov < > maxmu...@gmail.com > > >: > > >> > > > > > > > >> > > > > > > Hello, > > >> > > > > > > > > >> > > > > > > I've made some research about this problem and i think > that > > in > > >> > > > general > > >> > > > > we > > >> > > > > > > should move stopAllGrids method in GridAbstractTest class > to > > >> > > > > > > afterTestsStopped method with some changes. Am I right? > > >> > > > > > > > > >> > > > > > > Also, I have a question about stopAllGrids(boolean cancel) > > >> this > > >> > > > > "cancel" > > >> > > > > > > argument. Why in some cases we should interrupt ComputeJob > > >> and in > > >> > > > some > > >> > > > > > > cases shouldn't? For example here: > > >> > > > > > > IgniteBaselineAffinityTopologyActivationTest#afterTest > > >> > > > > > > we call method stopAllGrids(false) this way. Why not > "true" > > >> > > argument > > >> > > > > > > instead? > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > -- > > >> > > > > > Best regards, > > >> > > > > > Andrey Kuznetsov. > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > > > > >
