I agree it is important, I'm going to do a review, but do not have time slot to do.
Who could pick up this review? Dmitriy G., could I ask you? пн, 19 мар. 2018 г. в 15:13, Maxim Muzafarov <maxmu...@gmail.com>: > 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. >>> > > >> > > > > >> > > > > > >>> > > >> > > > > >> > > > > >>> > > >> > > > > >> > > > >>> > > >> > > > > >> > > >>> > > >> > > > > >> > >>> > > >> > > > > >> >>> > > >> > > > > > >>> > > >> > > > > >>> > > >> > > > >>> > > >> > > >>> > > >> > >>> > > >> >>> > > > >>> > > >>> > >>> >>
