Dmitry and other igniters, Will you have time to review this issue? I've preperated PR and TC for this, also I've fixed all comments made by Andrey Kuznetsov and Vyacheslav Daradur.
I think this is important issue and will make test framework more stable and clear. TC: https://ci.ignite.apache.org/viewLog.html?buildId=1138151 JIRA: https://issues.apache.org/jira/browse/IGNITE-6842 Upsource: https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502 PR: https://github.com/apache/ignite/pull/3542 чт, 15 мар. 2018 г. в 13:31, Maxim Muzafarov <maxmu...@gmail.com>: > Dmtry, > > Can we proceed with this change? > I've done with fixing review comments and tests that you mentioned before. > > TC: https://ci.ignite.apache.org/viewLog.html?buildId=1138151 > JIRA: https://issues.apache.org/jira/browse/IGNITE-6842 > Upsource: https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502 > PR: https://github.com/apache/ignite/pull/3542 > > > > вт, 6 мар. 2018 г. в 20:42, Dmitry Pavlov <dpavlov....@gmail.com>: > >> Ok, thank you. >> >> Please let me know when we can proceed with review >> https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502 >> >> >> вт, 6 мар. 2018 г. в 20:17, Maxim Muzafarov <maxmu...@gmail.com>: >> >> > Hello Dmitry, >> > >> > Yes, I've updated test classes as you metioned before. >> > Now i'm fixing review comments. Within next few days I'll prepare final >> > version of this PR. >> > >> > вт, 6 мар. 2018 г. в 20:12, Dmitry Pavlov <dpavlov....@gmail.com>: >> > >> > > Hi Maxim, >> > > >> > > are there any news on these test fails? >> > > >> > > Is issue ready for review? >> > > >> > > Sincerely, >> > > Dmitiry Pavlov >> > > >> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <dpavlov....@gmail.com>: >> > > >> > > > Hi, thank you! >> > > > >> > > > I've found several suspicious fails: such test fails have rate less >> > than >> > > > 1%, it is probably new failures. >> > > > >> > > > It would be great if we can fix it before merge. Could you address >> this >> > > > fails? >> > > > >> > > > Sincerely, >> > > > Dmitriy Pavlov >> > > > >> > > > IgniteCacheTestSuite5: IgniteCacheStoreCollectionTest.testStoreMap >> > (fail >> > > > rate 0,0%) >> > > > IgniteCacheTestSuite5: >> > > > CacheLateAffinityAssignmentTest.testDelayAssignmentClientJoin (fail >> > rate >> > > > 0,0%) >> > > > IgniteCacheWithIndexingTestSuite: >> > > > CacheRandomOperationsMultithreadedTest.testAtomicOffheapEviction >> (fail >> > > rate >> > > > 0,0%) >> > > > IgniteCacheWithIndexingTestSuite: >> > > > >> > CacheRandomOperationsMultithreadedTest.testAtomicOffheapEvictionIndexing >> > > > (fail rate 0,0%) >> > > > IgniteCacheWithIndexingTestSuite: >> > > > CacheRandomOperationsMultithreadedTest.testTxOffheapEviction (fail >> rate >> > > > 0,0%) >> > > > IgniteCacheWithIndexingTestSuite: >> > > > CacheRandomOperationsMultithreadedTest.testTxOffheapEvictionIndexing >> > > (fail >> > > > rate 0,0%) >> > > > >> > > > IgniteBinarySimpleNameMapperCacheFullApiTestSuite: >> > > > >> > > >> > >> GridCachePartitionedNearDisabledMultiNodeWithGroupFullApiSelfTest.testWithSkipStoreTx >> > > > (fail rate 0,0%) >> > > > >> > > > вт, 27 февр. 2018 г. в 17:04, Maxim Muzafarov <maxmu...@gmail.com>: >> > > > >> > > >> Yep, link may not be correct. >> > > >> >> > > >> Here is correct version: >> > > >> TC: * >> > > >> >> > > >> > >> https://ci.ignite.apache.org/project.html?projectId=IgniteTests24Java8&branch_IgniteTests24Java8=pull%2F3542%2Fhead >> > > >> < >> > > >> >> > > >> > >> https://ci.ignite.apache.org/project.html?projectId=IgniteTests24Java8&branch_IgniteTests24Java8=pull%2F3542%2Fhead >> > > >> >* >> > > >> >> > > >> >> > > >> вт, 27 февр. 2018 г. в 16:41, Dmitry Pavlov <dpavlov....@gmail.com >> >: >> > > >> >> > > >> > Hi Maxim, >> > > >> > >> > > >> > could you please provide link to TC run on your PR? It seems link >> > > >> provided >> > > >> > points to run of master. In changes field you may select >> > > pull/3542/head >> > > >> > before starting RunAll. >> > > >> > >> > > >> > Igniters, >> > > >> > >> > > >> > this change is related to our test framework, so change may >> affect >> > > your >> > > >> > tests. Please join to review >> > > >> > https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502 >> > > >> > >> > > >> > Sincerely, >> > > >> > Dmitriy Pavlov >> > > >> > >> > > >> > вт, 27 февр. 2018 г. в 16:14, Maxim Muzafarov < >> maxmu...@gmail.com>: >> > > >> > >> > > >> > > 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. >> > > >> > > > > >> > > > > > >> > > >> > > > > >> > > > > >> > > >> > > > > >> > > > >> > > >> > > > > >> > > >> > > >> > > > > >> > >> > > >> > > > > >> >> > > >> > > > > > >> > > >> > > > > >> > > >> > > > >> > > >> > > >> > > >> > >> > > >> >> > > > >> > > >> > >> >
