Stop nodes after test by default - IGNITE-6842

[Mmuzaf]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > Hi Maxim,
> > >
> > > are there any news on these test fails?
> > >
> > > Is issue ready for review?
> > >
> > > Sincerely,
> > > Dmitiry Pavlov
> > >
> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]>:
> > >
> > > > 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 <[hidden email]>:
> > > >
> > > >> 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 <[hidden email]
> >:
> > > >>
> > > >> > 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 <[hidden email]
> >:
> > > >> >
> > > >> > > 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 <
> > [hidden email]
> > > >:
> > > >> > >
> > > >> > > > 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 <
> > [hidden email]
> > > >:
> > > >> > > >
> > > >> > > > > 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 <
> > > [hidden email]
> > > >> >:
> > > >> > > > >
> > > >> > > > > > Thank you all,
> > > >> > > > > >
> > > >> > > > > > I'll add this comment's for JIRA ticket, if you don't
> mind.
> > > >> > > > > >
> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
> > > >> [hidden email]
> > > >> > >:
> > > >> > > > > >
> > > >> > > > > >> 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 <
> > > >> > > > [hidden email]
> > > >> > > > > >:
> > > >> > > > > >>
> > > >> > > > > >> > 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 <
> > > >> > > > [hidden email]>
> > > >> > > > > >> > 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 <
> > > >> > > > > >> [hidden email]
> > > >> > > > > >> > >:
> > > >> > > > > >> > >
> > > >> > > > > >> > > > 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 <
> > > >> > > > > >> [hidden email]>
> > > >> > > > > >> > > > 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 <
> > > >> > > > > [hidden email]
> > > >> > > > > >> >:
> > > >> > > > > >> > > > >
> > > >> > > > > >> > > > > > 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 <
> > > >> > > > [hidden email]
> > > >> > > > > >:
> > > >> > > > > >> > > > > >
> > > >> > > > > >> > > > > > > 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.
> > > >> > > > > >> > > > > >
> > > >> > > > > >> > > > >
> > > >> > > > > >> > > >
> > > >> > > > > >> > >
> > > >> > > > > >> >
> > > >> > > > > >>
> > > >> > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>

[Mmuzaf]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
>> 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 <[hidden email]>:
>>
>> > 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 <[hidden email]>:
>> >
>> > > Hi Maxim,
>> > >
>> > > are there any news on these test fails?
>> > >
>> > > Is issue ready for review?
>> > >
>> > > Sincerely,
>> > > Dmitiry Pavlov
>> > >
>> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]>:
>> > >
>> > > > 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 <[hidden email]>:
>> > > >
>> > > >> 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 <[hidden email]
>> >:
>> > > >>
>> > > >> > 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 <
>> [hidden email]>:
>> > > >> >
>> > > >> > > 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 <
>> > [hidden email]
>> > > >:
>> > > >> > >
>> > > >> > > > 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 <
>> > [hidden email]
>> > > >:
>> > > >> > > >
>> > > >> > > > > 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 <
>> > > [hidden email]
>> > > >> >:
>> > > >> > > > >
>> > > >> > > > > > Thank you all,
>> > > >> > > > > >
>> > > >> > > > > > I'll add this comment's for JIRA ticket, if you don't
>> mind.
>> > > >> > > > > >
>> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
>> > > >> [hidden email]
>> > > >> > >:
>> > > >> > > > > >
>> > > >> > > > > >> 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 <
>> > > >> > > > [hidden email]
>> > > >> > > > > >:
>> > > >> > > > > >>
>> > > >> > > > > >> > 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 <
>> > > >> > > > [hidden email]>
>> > > >> > > > > >> > 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 <
>> > > >> > > > > >> [hidden email]
>> > > >> > > > > >> > >:
>> > > >> > > > > >> > >
>> > > >> > > > > >> > > > 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 <
>> > > >> > > > > >> [hidden email]>
>> > > >> > > > > >> > > > 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 <
>> > > >> > > > > [hidden email]
>> > > >> > > > > >> >:
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > > > > 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 <
>> > > >> > > > [hidden email]
>> > > >> > > > > >:
>> > > >> > > > > >> > > > > >
>> > > >> > > > > >> > > > > > > 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.
>> > > >> > > > > >> > > > > >
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > >
>> > > >> > > > > >> >
>> > > >> > > > > >>
>> > > >> > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >> >
>> > > >>
>> > > >
>> > >
>> >
>>
>

[Dmitriy Pavlov]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
>> 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 <[hidden email]>:
>>
>>> 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 <[hidden email]>:
>>>
>>> > 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 <[hidden email]>:
>>> >
>>> > > Hi Maxim,
>>> > >
>>> > > are there any news on these test fails?
>>> > >
>>> > > Is issue ready for review?
>>> > >
>>> > > Sincerely,
>>> > > Dmitiry Pavlov
>>> > >
>>> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]>:
>>> > >
>>> > > > 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 <[hidden email]
>>> >:
>>> > > >
>>> > > >> 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 <
>>> [hidden email]>:
>>> > > >>
>>> > > >> > 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 <
>>> [hidden email]>:
>>> > > >> >
>>> > > >> > > 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 <
>>> > [hidden email]
>>> > > >:
>>> > > >> > >
>>> > > >> > > > 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 <
>>> > [hidden email]
>>> > > >:
>>> > > >> > > >
>>> > > >> > > > > 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 <
>>> > > [hidden email]
>>> > > >> >:
>>> > > >> > > > >
>>> > > >> > > > > > Thank you all,
>>> > > >> > > > > >
>>> > > >> > > > > > I'll add this comment's for JIRA ticket, if you don't
>>> mind.
>>> > > >> > > > > >
>>> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
>>> > > >> [hidden email]
>>> > > >> > >:
>>> > > >> > > > > >
>>> > > >> > > > > >> 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 <
>>> > > >> > > > [hidden email]
>>> > > >> > > > > >:
>>> > > >> > > > > >>
>>> > > >> > > > > >> > 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 <
>>> > > >> > > > [hidden email]>
>>> > > >> > > > > >> > 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 <
>>> > > >> > > > > >> [hidden email]
>>> > > >> > > > > >> > >:
>>> > > >> > > > > >> > >
>>> > > >> > > > > >> > > > 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 <
>>> > > >> > > > > >> [hidden email]>
>>> > > >> > > > > >> > > > 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 <
>>> > > >> > > > > [hidden email]
>>> > > >> > > > > >> >:
>>> > > >> > > > > >> > > > >
>>> > > >> > > > > >> > > > > > 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 <
>>> > > >> > > > [hidden email]
>>> > > >> > > > > >:
>>> > > >> > > > > >> > > > > >
>>> > > >> > > > > >> > > > > > > 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.
>>> > > >> > > > > >> > > > > >
>>> > > >> > > > > >> > > > >
>>> > > >> > > > > >> > > >
>>> > > >> > > > > >> > >
>>> > > >> > > > > >> >
>>> > > >> > > > > >>
>>> > > >> > > > > >
>>> > > >> > > > >
>>> > > >> > > >
>>> > > >> > >
>>> > > >> >
>>> > > >>
>>> > > >
>>> > >
>>> >
>>>
>>

[Dmitriy Govorukhin]

Looks good for me, please merge.

19 мар. 2018 г. 3:22 ПП пользователь "Dmitry Pavlov" <[hidden email]>
написал:

> 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 <[hidden email]>:
>
>> 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 <[hidden email]>:
>>
>>> 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 <[hidden email]>:
>>>
>>>> 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 <[hidden email]>:
>>>>
>>>> > 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 <[hidden email]>:
>>>> >
>>>> > > Hi Maxim,
>>>> > >
>>>> > > are there any news on these test fails?
>>>> > >
>>>> > > Is issue ready for review?
>>>> > >
>>>> > > Sincerely,
>>>> > > Dmitiry Pavlov
>>>> > >
>>>> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]
>>>> >:
>>>> > >
>>>> > > > 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.testAtomicOffheapEvictionIndex
>>>> ing
>>>> > > > (fail rate 0,0%)
>>>> > > > IgniteCacheWithIndexingTestSuite:
>>>> > > > CacheRandomOperationsMultithreadedTest.testTxOffheapEviction
>>>> (fail rate
>>>> > > > 0,0%)
>>>> > > > IgniteCacheWithIndexingTestSuite:
>>>> > > > CacheRandomOperationsMultithreadedTest.
>>>> testTxOffheapEvictionIndexing
>>>> > > (fail
>>>> > > > rate 0,0%)
>>>> > > >
>>>> > > > IgniteBinarySimpleNameMapperCacheFullApiTestSuite:
>>>> > > >
>>>> > >
>>>> > GridCachePartitionedNearDisabledMultiNodeWithGroupFullApiSel
>>>> fTest.testWithSkipStoreTx
>>>> > > > (fail rate 0,0%)
>>>> > > >
>>>> > > > вт, 27 февр. 2018 г. в 17:04, Maxim Muzafarov <[hidden email]
>>>> >:
>>>> > > >
>>>> > > >> 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 <
>>>> [hidden email]>:
>>>> > > >>
>>>> > > >> > 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 <
>>>> [hidden email]>:
>>>> > > >> >
>>>> > > >> > > 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 <
>>>> > [hidden email]
>>>> > > >:
>>>> > > >> > >
>>>> > > >> > > > 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 <
>>>> > [hidden email]
>>>> > > >:
>>>> > > >> > > >
>>>> > > >> > > > > 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 <
>>>> > > [hidden email]
>>>> > > >> >:
>>>> > > >> > > > >
>>>> > > >> > > > > > Thank you all,
>>>> > > >> > > > > >
>>>> > > >> > > > > > I'll add this comment's for JIRA ticket, if you don't
>>>> mind.
>>>> > > >> > > > > >
>>>> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
>>>> > > >> [hidden email]
>>>> > > >> > >:
>>>> > > >> > > > > >
>>>> > > >> > > > > >> 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 <
>>>> > > >> > > > [hidden email]
>>>> > > >> > > > > >:
>>>> > > >> > > > > >>
>>>> > > >> > > > > >> > 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 <
>>>> > > >> > > > [hidden email]>
>>>> > > >> > > > > >> > 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 <
>>>> > > >> > > > > >> [hidden email]
>>>> > > >> > > > > >> > >:
>>>> > > >> > > > > >> > >
>>>> > > >> > > > > >> > > > 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 <
>>>> > > >> > > > > >> [hidden email]>
>>>> > > >> > > > > >> > > > 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
>>>> <
>>>> > > >> > > > > [hidden email]
>>>> > > >> > > > > >> >:
>>>> > > >> > > > > >> > > > >
>>>> > > >> > > > > >> > > > > > 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 <
>>>> > > >> > > > [hidden email]
>>>> > > >> > > > > >:
>>>> > > >> > > > > >> > > > > >
>>>> > > >> > > > > >> > > > > > > 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.
>>>> > > >> > > > > >> > > > > >
>>>> > > >> > > > > >> > > > >
>>>> > > >> > > > > >> > > >
>>>> > > >> > > > > >> > >
>>>> > > >> > > > > >> >
>>>> > > >> > > > > >>
>>>> > > >> > > > > >
>>>> > > >> > > > >
>>>> > > >> > > >
>>>> > > >> > >
>>>> > > >> >
>>>> > > >>
>>>> > > >
>>>> > >
>>>> >
>>>>
>>>

[Dmitriy Pavlov]

Dmitriy, thank you for review. This fix should do our tests more stable.

Nikolay, could you please merge?

вт, 20 мар. 2018 г. в 11:49, Dmitriy Govorukhin <
[hidden email]>:

> Looks good for me, please merge.
>
> 19 мар. 2018 г. 3:22 ПП пользователь "Dmitry Pavlov" <
> [hidden email]> написал:
>
> 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 <[hidden email]>:
>>
>>> 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 <[hidden email]>:
>>>
>>>> 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 <[hidden email]>:
>>>>
>>>>> 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 <[hidden email]>:
>>>>>
>>>>> > 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 <[hidden email]>:
>>>>> >
>>>>> > > Hi Maxim,
>>>>> > >
>>>>> > > are there any news on these test fails?
>>>>> > >
>>>>> > > Is issue ready for review?
>>>>> > >
>>>>> > > Sincerely,
>>>>> > > Dmitiry Pavlov
>>>>> > >
>>>>> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]
>>>>> >:
>>>>> > >
>>>>> > > > 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 <
>>>>> [hidden email]>:
>>>>> > > >
>>>>> > > >> 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 <
>>>>> [hidden email]>:
>>>>> > > >>
>>>>> > > >> > 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 <
>>>>> [hidden email]>:
>>>>> > > >> >
>>>>> > > >> > > 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 <
>>>>> > [hidden email]
>>>>> > > >:
>>>>> > > >> > >
>>>>> > > >> > > > 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 <
>>>>> > [hidden email]
>>>>> > > >:
>>>>> > > >> > > >
>>>>> > > >> > > > > 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 <
>>>>> > > [hidden email]
>>>>> > > >> >:
>>>>> > > >> > > > >
>>>>> > > >> > > > > > Thank you all,
>>>>> > > >> > > > > >
>>>>> > > >> > > > > > I'll add this comment's for JIRA ticket, if you don't
>>>>> mind.
>>>>> > > >> > > > > >
>>>>> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
>>>>> > > >> [hidden email]
>>>>> > > >> > >:
>>>>> > > >> > > > > >
>>>>> > > >> > > > > >> 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 <
>>>>> > > >> > > > [hidden email]
>>>>> > > >> > > > > >:
>>>>> > > >> > > > > >>
>>>>> > > >> > > > > >> > 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 <
>>>>> > > >> > > > [hidden email]>
>>>>> > > >> > > > > >> > 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 <
>>>>> > > >> > > > > >> [hidden email]
>>>>> > > >> > > > > >> > >:
>>>>> > > >> > > > > >> > >
>>>>> > > >> > > > > >> > > > 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 <
>>>>> > > >> > > > > >> [hidden email]>
>>>>> > > >> > > > > >> > > > 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 <
>>>>> > > >> > > > > [hidden email]
>>>>> > > >> > > > > >> >:
>>>>> > > >> > > > > >> > > > >
>>>>> > > >> > > > > >> > > > > > 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 <
>>>>> > > >> > > > [hidden email]
>>>>> > > >> > > > > >:
>>>>> > > >> > > > > >> > > > > >
>>>>> > > >> > > > > >> > > > > > > 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.
>>>>> > > >> > > > > >> > > > > >
>>>>> > > >> > > > > >> > > > >
>>>>> > > >> > > > > >> > > >
>>>>> > > >> > > > > >> > >
>>>>> > > >> > > > > >> >
>>>>> > > >> > > > > >>
>>>>> > > >> > > > > >
>>>>> > > >> > > > >
>>>>> > > >> > > >
>>>>> > > >> > >
>>>>> > > >> >
>>>>> > > >>
>>>>> > > >
>>>>> > >
>>>>> >
>>>>>
>>>>

[Dmitriy Pavlov]

Hi Nikolai, will you have time to merge this change?

вт, 20 мар. 2018 г. в 11:52, Dmitry Pavlov <[hidden email]>:

> Dmitriy, thank you for review. This fix should do our tests more stable.
>
> Nikolay, could you please merge?
>
> вт, 20 мар. 2018 г. в 11:49, Dmitriy Govorukhin <
> [hidden email]>:
>
>> Looks good for me, please merge.
>>
>> 19 мар. 2018 г. 3:22 ПП пользователь "Dmitry Pavlov" <
>> [hidden email]> написал:
>>
>> 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 <[hidden email]>:
>>>
>>>> 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 <[hidden email]>:
>>>>
>>>>> 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 <[hidden email]>:
>>>>>
>>>>>> 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 <[hidden email]>:
>>>>>>
>>>>>> > 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 <[hidden email]>:
>>>>>> >
>>>>>> > > Hi Maxim,
>>>>>> > >
>>>>>> > > are there any news on these test fails?
>>>>>> > >
>>>>>> > > Is issue ready for review?
>>>>>> > >
>>>>>> > > Sincerely,
>>>>>> > > Dmitiry Pavlov
>>>>>> > >
>>>>>> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <
>>>>>> [hidden email]>:
>>>>>> > >
>>>>>> > > > 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 <
>>>>>> [hidden email]>:
>>>>>> > > >
>>>>>> > > >> 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 <
>>>>>> [hidden email]>:
>>>>>> > > >>
>>>>>> > > >> > 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 <
>>>>>> [hidden email]>:
>>>>>> > > >> >
>>>>>> > > >> > > 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 <
>>>>>> > [hidden email]
>>>>>> > > >:
>>>>>> > > >> > >
>>>>>> > > >> > > > 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 <
>>>>>> > [hidden email]
>>>>>> > > >:
>>>>>> > > >> > > >
>>>>>> > > >> > > > > 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 <
>>>>>> > > [hidden email]
>>>>>> > > >> >:
>>>>>> > > >> > > > >
>>>>>> > > >> > > > > > Thank you all,
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > > > I'll add this comment's for JIRA ticket, if you
>>>>>> don't mind.
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
>>>>>> > > >> [hidden email]
>>>>>> > > >> > >:
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > > >> 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 <
>>>>>> > > >> > > > [hidden email]
>>>>>> > > >> > > > > >:
>>>>>> > > >> > > > > >>
>>>>>> > > >> > > > > >> > 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 <
>>>>>> > > >> > > > [hidden email]>
>>>>>> > > >> > > > > >> > 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 <
>>>>>> > > >> > > > > >> [hidden email]
>>>>>> > > >> > > > > >> > >:
>>>>>> > > >> > > > > >> > >
>>>>>> > > >> > > > > >> > > > 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
>>>>>> <
>>>>>> > > >> > > > > >> [hidden email]>
>>>>>> > > >> > > > > >> > > > 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 <
>>>>>> > > >> > > > > [hidden email]
>>>>>> > > >> > > > > >> >:
>>>>>> > > >> > > > > >> > > > >
>>>>>> > > >> > > > > >> > > > > > 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
>>>>>> <
>>>>>> > > >> > > > [hidden email]
>>>>>> > > >> > > > > >:
>>>>>> > > >> > > > > >> > > > > >
>>>>>> > > >> > > > > >> > > > > > > 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.
>>>>>> > > >> > > > > >> > > > > >
>>>>>> > > >> > > > > >> > > > >
>>>>>> > > >> > > > > >> > > >
>>>>>> > > >> > > > > >> > >
>>>>>> > > >> > > > > >> >
>>>>>> > > >> > > > > >>
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > >
>>>>>> > > >> > > >
>>>>>> > > >> > >
>>>>>> > > >> >
>>>>>> > > >>
>>>>>> > > >
>>>>>> > >
>>>>>> >
>>>>>
>>>>> ikolai, would you have time to merge this change?

вт, 20 мар. 2018 г. в 11:52, Dmitry Pavlov <[hidden email]>:

> Dmitriy, thank you for review. This fix should do our tests more stable.
>
> Nikolay, could you please merge?
>
> вт, 20 мар. 2018 г. в 11:49, Dmitriy Govorukhin <
> [hidden email]>:
>
>> Looks good for me, please merge.
>>
>> 19 мар. 2018 г. 3:22 ПП пользователь "Dmitry Pavlov" <
>> [hidden email]> написал:
>>
>> 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 <[hidden email]>:
>>>
>>>> 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 <[hidden email]>:
>>>>
>>>>> 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 <[hidden email]>:
>>>>>
>>>>>> 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 <[hidden email]>:
>>>>>>
>>>>>> > 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 <[hidden email]>:
>>>>>> >
>>>>>> > > Hi Maxim,
>>>>>> > >
>>>>>> > > are there any news on these test fails?
>>>>>> > >
>>>>>> > > Is issue ready for review?
>>>>>> > >
>>>>>> > > Sincerely,
>>>>>> > > Dmitiry Pavlov
>>>>>> > >
>>>>>> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <
>>>>>> [hidden email]>:
>>>>>> > >
>>>>>> > > > 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 <
>>>>>> [hidden email]>:
>>>>>> > > >
>>>>>> > > >> 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 <
>>>>>> [hidden email]>:
>>>>>> > > >>
>>>>>> > > >> > 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 <
>>>>>> [hidden email]>:
>>>>>> > > >> >
>>>>>> > > >> > > 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 <
>>>>>> > [hidden email]
>>>>>> > > >:
>>>>>> > > >> > >
>>>>>> > > >> > > > 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 <
>>>>>> > [hidden email]
>>>>>> > > >:
>>>>>> > > >> > > >
>>>>>> > > >> > > > > 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 <
>>>>>> > > [hidden email]
>>>>>> > > >> >:
>>>>>> > > >> > > > >
>>>>>> > > >> > > > > > Thank you all,
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > > > I'll add this comment's for JIRA ticket, if you
>>>>>> don't mind.
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
>>>>>> > > >> [hidden email]
>>>>>> > > >> > >:
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > > >> 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 <
>>>>>> > > >> > > > [hidden email]
>>>>>> > > >> > > > > >:
>>>>>> > > >> > > > > >>
>>>>>> > > >> > > > > >> > 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 <
>>>>>> > > >> > > > [hidden email]>
>>>>>> > > >> > > > > >> > 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 <
>>>>>> > > >> > > > > >> [hidden email]
>>>>>> > > >> > > > > >> > >:
>>>>>> > > >> > > > > >> > >
>>>>>> > > >> > > > > >> > > > 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
>>>>>> <
>>>>>> > > >> > > > > >> [hidden email]>
>>>>>> > > >> > > > > >> > > > 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 <
>>>>>> > > >> > > > > [hidden email]
>>>>>> > > >> > > > > >> >:
>>>>>> > > >> > > > > >> > > > >
>>>>>> > > >> > > > > >> > > > > > 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
>>>>>> <
>>>>>> > > >> > > > [hidden email]
>>>>>> > > >> > > > > >:
>>>>>> > > >> > > > > >> > > > > >
>>>>>> > > >> > > > > >> > > > > > > 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.
>>>>>> > > >> > > > > >> > > > > >
>>>>>> > > >> > > > > >> > > > >
>>>>>> > > >> > > > > >> > > >
>>>>>> > > >> > > > > >> > >
>>>>>> > > >> > > > > >> >
>>>>>> > > >> > > > > >>
>>>>>> > > >> > > > > >
>>>>>> > > >> > > > >
>>>>>> > > >> > > >
>>>>>> > > >> > >
>>>>>> > > >> >
>>>>>> > > >>
>>>>>> > > >
>>>>>> > >
>>>>>> >
>>>>>>
>>>>>