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. >> > > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > > > >> > > > > >> > > >> > > > > >> > >> > > > > >> >> > > > > > >> > > > > >> > > > >> > > >> > >> >
