ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Maxim Muzafarov <maxmu...@gmail.com>
Subject Re: Stop nodes after test by default - IGNITE-6842
Date Mon, 19 Mar 2018 12:13:09 GMT
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 <maxmuzaf@gmail.com>:

> Dmtry,
>
> Can we proceed with this change?
> I've done with fixing review comments and tests that you mentioned before.
>
> TC: https://ci.ignite.apache.org/viewLog.html?buildId=1138151
> JIRA: https://issues.apache.org/jira/browse/IGNITE-6842
> Upsource: https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502
> PR: https://github.com/apache/ignite/pull/3542
>
>
>
> вт, 6 мар. 2018 г. в 20:42, Dmitry Pavlov <dpavlov.spb@gmail.com>:
>
>> Ok, thank you.
>>
>> Please let me know when we can proceed with review
>> https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502
>>
>>
>> вт, 6 мар. 2018 г. в 20:17, Maxim Muzafarov <maxmuzaf@gmail.com>:
>>
>> > Hello Dmitry,
>> >
>> > Yes, I've updated test classes as you metioned before.
>> > Now i'm fixing review comments. Within next few days I'll prepare final
>> > version of this PR.
>> >
>> > вт, 6 мар. 2018 г. в 20:12, Dmitry Pavlov <dpavlov.spb@gmail.com>:
>> >
>> > > Hi Maxim,
>> > >
>> > > are there any news on these test fails?
>> > >
>> > > Is issue ready for review?
>> > >
>> > > Sincerely,
>> > > Dmitiry Pavlov
>> > >
>> > > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <dpavlov.spb@gmail.com>:
>> > >
>> > > > Hi, thank you!
>> > > >
>> > > > I've found several suspicious fails: such test fails have rate less
>> > than
>> > > > 1%, it is probably new failures.
>> > > >
>> > > > It would be great if we can fix it before merge. Could you address
>> this
>> > > > fails?
>> > > >
>> > > > Sincerely,
>> > > > Dmitriy Pavlov
>> > > >
>> > > > IgniteCacheTestSuite5: IgniteCacheStoreCollectionTest.testStoreMap
>> > (fail
>> > > > rate 0,0%)
>> > > > IgniteCacheTestSuite5:
>> > > > CacheLateAffinityAssignmentTest.testDelayAssignmentClientJoin (fail
>> > rate
>> > > > 0,0%)
>> > > > IgniteCacheWithIndexingTestSuite:
>> > > > CacheRandomOperationsMultithreadedTest.testAtomicOffheapEviction
>> (fail
>> > > rate
>> > > > 0,0%)
>> > > > IgniteCacheWithIndexingTestSuite:
>> > > >
>> > CacheRandomOperationsMultithreadedTest.testAtomicOffheapEvictionIndexing
>> > > > (fail rate 0,0%)
>> > > > IgniteCacheWithIndexingTestSuite:
>> > > > CacheRandomOperationsMultithreadedTest.testTxOffheapEviction (fail
>> rate
>> > > > 0,0%)
>> > > > IgniteCacheWithIndexingTestSuite:
>> > > > CacheRandomOperationsMultithreadedTest.testTxOffheapEvictionIndexing
>> > > (fail
>> > > > rate 0,0%)
>> > > >
>> > > > IgniteBinarySimpleNameMapperCacheFullApiTestSuite:
>> > > >
>> > >
>> >
>> GridCachePartitionedNearDisabledMultiNodeWithGroupFullApiSelfTest.testWithSkipStoreTx
>> > > > (fail rate 0,0%)
>> > > >
>> > > > вт, 27 февр. 2018 г. в 17:04, Maxim Muzafarov <maxmuzaf@gmail.com>:
>> > > >
>> > > >> Yep, link may not be correct.
>> > > >>
>> > > >> Here is correct version:
>> > > >> TC: *
>> > > >>
>> > >
>> >
>> https://ci.ignite.apache.org/project.html?projectId=IgniteTests24Java8&branch_IgniteTests24Java8=pull%2F3542%2Fhead
>> > > >> <
>> > > >>
>> > >
>> >
>> https://ci.ignite.apache.org/project.html?projectId=IgniteTests24Java8&branch_IgniteTests24Java8=pull%2F3542%2Fhead
>> > > >> >*
>> > > >>
>> > > >>
>> > > >> вт, 27 февр. 2018 г. в 16:41, Dmitry Pavlov <dpavlov.spb@gmail.com
>> >:
>> > > >>
>> > > >> > Hi Maxim,
>> > > >> >
>> > > >> > could you please provide link to TC run on your PR? It seems
link
>> > > >> provided
>> > > >> > points to run of master. In changes field you may select
>> > > pull/3542/head
>> > > >> > before starting RunAll.
>> > > >> >
>> > > >> > Igniters,
>> > > >> >
>> > > >> > this change is related to our test framework, so change may
>> affect
>> > > your
>> > > >> > tests. Please join to review
>> > > >> > https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502
>> > > >> >
>> > > >> > Sincerely,
>> > > >> > Dmitriy Pavlov
>> > > >> >
>> > > >> > вт, 27 февр. 2018 г. в 16:14, Maxim Muzafarov <
>> maxmuzaf@gmail.com>:
>> > > >> >
>> > > >> > > Hi all,
>> > > >> > >
>> > > >> > > I think, I've done with this issue, what should we do
next?
>> > > >> > >
>> > > >> > > PR: https://github.com/apache/ignite/pull/3542
>> > > >> > > Upsource:
>> > > https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502
>> > > >> > > TC:
>> > > >> > >
>> > > >> > >
>> > > >> >
>> > > >>
>> > >
>> >
>> https://ci.ignite.apache.org/viewModification.html?modId=723895&personal=false&buildTypeId=&tab=vcsModificationTests
>> > > >> > > JIRA: https://issues.apache.org/jira/browse/IGNITE-6842
>> > > >> > >
>> > > >> > >
>> > > >> > > чт, 22 февр. 2018 г. в 14:12, Dmitry Pavlov
<
>> > dpavlov.spb@gmail.com
>> > > >:
>> > > >> > >
>> > > >> > > > Hi Maxim,
>> > > >> > > >
>> > > >> > > > Thank you.
>> > > >> > > >
>> > > >> > > > To my mind stopAllGrids call should be kept in
>> afterTestsStop().
>> > > If
>> > > >> > > > developer forgot to call super(), he will see exception
from
>> > > >> > > > beforeTestsStart()
>> > > >> > > > added by you.
>> > > >> > > >
>> > > >> > > > If you think patch is ready to be reviewed, please
change
>> JIRA
>> > > >> status
>> > > >> > to
>> > > >> > > > "Patch Available".
>> > > >> > > >
>> > > >> > > > Optionally you can create Upsource review. It would
be easier
>> > for
>> > > >> > > multiple
>> > > >> > > > reviewers to handle this patch. This test framework
is used
>> by
>> > all
>> > > >> > > Igniters
>> > > >> > > > so Upsource would be good option (
>> > > >> https://reviews.ignite.apache.org/
>> > > >> > ).
>> > > >> > > >
>> > > >> > > > Sincerely,
>> > > >> > > > Dmitriy Pavlov
>> > > >> > > >
>> > > >> > > > чт, 22 февр. 2018 г. в 13:44, Maxim Muzafarov
<
>> > maxmuzaf@gmail.com
>> > > >:
>> > > >> > > >
>> > > >> > > > > Hi all,
>> > > >> > > > >
>> > > >> > > > > I've made some changes based on our previous
discusstions,
>> > > please
>> > > >> see
>> > > >> > > PR
>> > > >> > > > > [1]:
>> > > >> > > > > 1) Remove duplicated code for stopGrid (by
index and by
>> name);
>> > > >> > > > > 2) Add new method that thows exception if
not all grids
>> > haven't
>> > > >> > stopped
>> > > >> > > > > correctly;
>> > > >> > > > > 3)  Change tests that have been affected by
this changes;
>> > > >> > > > >
>> > > >> > > > > Also, I have some thoughts for clarification:
>> > > >> > > > > 1) beforeTestsStart() - I expect here in subtests
that
>> grids
>> > are
>> > > >> not
>> > > >> > > > > started yet. Am I right?
>> > > >> > > > > 2) I think we should call stopAllGrids in
tearDown method.
>> So,
>> > > if
>> > > >> in
>> > > >> > > some
>> > > >> > > > > cases we'll override afterTestsStop in subclasses
and
>> forgot
>> > to
>> > > >> call
>> > > >> > > > > super() it won't lead us to exception.
>> > > >> > > > >
>> > > >> > > > > [1] https://github.com/apache/ignite/pull/3542
>> > > >> > > > > [2]
>> > > >> https://ci.ignite.apache.org/viewModification.html?modId=717275
>> > > >> > > > > [3] https://issues.apache.org/jira/browse/IGNITE-6842
>> > > >> > > > >
>> > > >> > > > >
>> > > >> > > > > ср, 7 февр. 2018 г. в 18:28, Maxim
Muzafarov <
>> > > maxmuzaf@gmail.com
>> > > >> >:
>> > > >> > > > >
>> > > >> > > > > > Thank you all,
>> > > >> > > > > >
>> > > >> > > > > > I'll add this comment's for JIRA ticket,
if you don't
>> mind.
>> > > >> > > > > >
>> > > >> > > > > > ср, 7 февр. 2018 г. в 15:16,
Dmitry Pavlov <
>> > > >> dpavlov.spb@gmail.com
>> > > >> > >:
>> > > >> > > > > >
>> > > >> > > > > >> Yes, this solution allows to cover
both cases:
>> > > >> > > > > >> a) not stopped node from previous
test and
>> > > >> > > > > >> b) allows to remove useless code
that stops Ignite nodes
>> > from
>> > > >> each
>> > > >> > > > test.
>> > > >> > > > > >>
>> > > >> > > > > >> ср, 7 февр. 2018 г. в 15:13,
Anton Vinogradov <
>> > > >> > > > avinogradov@gridgain.com
>> > > >> > > > > >:
>> > > >> > > > > >>
>> > > >> > > > > >> > Maxim,
>> > > >> > > > > >> >
>> > > >> > > > > >> > We discussed with Dima privately,
and decided
>> > > >> > > > > >> >
>> > > >> > > > > >> > 1) We have to assert that there
is no alive nodes at
>> > > >> > > > > GridAbstractTest's
>> > > >> > > > > >> > beforeTestsStarted
>> > > >> > > > > >> > 2) We have to kill all alive
nodes (without force) at
>> > > >> > > > > GridAbstractTest's
>> > > >> > > > > >> > afterTestsStopped
>> > > >> > > > > >> > 3) In case of any exceptions
at #2 we have to see test
>> > > error
>> > > >> > > > > >> > 4) We can get rid of all useless
stopAllGrids at
>> > > >> > > GridAbstractTest's
>> > > >> > > > > >> > subclasses.
>> > > >> > > > > >> >
>> > > >> > > > > >> >
>> > > >> > > > > >> >
>> > > >> > > > > >> > On Wed, Feb 7, 2018 at 2:32
PM, Dmitry Pavlov <
>> > > >> > > > dpavlov.spb@gmail.com>
>> > > >> > > > > >> > wrote:
>> > > >> > > > > >> >
>> > > >> > > > > >> > > > Let's just add stopAllGrids(flase)
to
>> > GridAbstractTest
>> > > 's
>> > > >> > > > > >> > > afterTestsStopped
>> > > >> > > > > >> > > method body.
>> > > >> > > > > >> > >
>> > > >> > > > > >> > > Can't agree with it becase
implicit silent shutdown
>> of
>> > > >> nodes
>> > > >> > > from
>> > > >> > > > > test
>> > > >> > > > > >> > > framework may cause a lot
of bugs introduced to
>> Ignite.
>> > > >> > > > > >> > >
>> > > >> > > > > >> > > I think stop+test fail
should occur.
>> > > >> > > > > >> > > In that case author of
incorrect test or not
>> consistent
>> > > >> Ignite
>> > > >> > > > > >> shutdown
>> > > >> > > > > >> > > will see problem.
>> > > >> > > > > >> > >
>> > > >> > > > > >> > > 'Fail fast' if always better
than hidding real
>> problem
>> > > >> under
>> > > >> > > > > automatic
>> > > >> > > > > >> > > framework feature.
>> > > >> > > > > >> > >
>> > > >> > > > > >> > > ср, 7 февр. 2018
г. в 14:05, Anton Vinogradov <
>> > > >> > > > > >> avinogradov@gridgain.com
>> > > >> > > > > >> > >:
>> > > >> > > > > >> > >
>> > > >> > > > > >> > > > Hi all,
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > > > > I've made some
research about this problem and i
>> > > think
>> > > >> > that
>> > > >> > > in
>> > > >> > > > > >> > general
>> > > >> > > > > >> > > we
>> > > >> > > > > >> > > > > should move stopAllGrids
method in
>> GridAbstractTest
>> > > >> class
>> > > >> > to
>> > > >> > > > > >> > > > > afterTestsStopped
method with some changes. Am I
>> > > right?
>> > > >> > > > > >> > > > Let's just add stopAllGrids(flase)
to
>> > GridAbstractTest
>> > > 's
>> > > >> > > > > >> > > > afterTestsStopped
method
>> > > >> > > > > >> > > > body.
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > > > > I have a question
about stopAllGrids(boolean
>> > cancel)
>> > > >> this
>> > > >> > > > > "cancel"
>> > > >> > > > > >> > > > That's just a flag
means "do not wait for any
>> > > operations
>> > > >> > > finish"
>> > > >> > > > > >> > > > See no reason to set
it true in that case.
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > > > > If you delegate
closing to afterTestsStopped
>> this
>> > > will
>> > > >> > > affect
>> > > >> > > > > only
>> > > >> > > > > >> > > > > last test (method).
>> > > >> > > > > >> > > > The idea is to stop
all nodes at last test's
>> method
>> > > >> finish.
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > > > >  Nodes that survive
between tests can affect
>> > > successive
>> > > >> > > > > >> > > > tests.
>> > > >> > > > > >> > > > Thats ok. we have
a lot tests where nodes reusable
>> > > >> between
>> > > >> > > > test's
>> > > >> > > > > >> > > methods.
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > > > On Wed, Feb 7, 2018
at 1:20 PM, Dmitry Pavlov <
>> > > >> > > > > >> dpavlov.spb@gmail.com>
>> > > >> > > > > >> > > > wrote:
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > > > > Hi Igniters,
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > > > IMO nodes that
survive between tests is not
>> general
>> > > >> > practice
>> > > >> > > > and
>> > > >> > > > > >> I'm
>> > > >> > > > > >> > > not
>> > > >> > > > > >> > > > > sure is a best
practice. I suggest to mark such
>> > tests
>> > > >> with
>> > > >> > > > some
>> > > >> > > > > >> > method
>> > > >> > > > > >> > > > > overriden from
AbstractTest.
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > > > About cancel
flag, please note
>> stopAllGrids(boolean
>> > > >> > cancel)
>> > > >> > > > > >> > > cancel=false
>> > > >> > > > > >> > > > > allows to wait
of checkpoint ends in case
>> > persistence
>> > > >> > > enabled.
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > > > I still suggest
to avoid stopping any nodes by
>> > test,
>> > > >> but
>> > > >> > > > > validate
>> > > >> > > > > >> not
>> > > >> > > > > >> > > > > stopped node
exist and fail test instead of
>> siltent
>> > > >> > implicit
>> > > >> > > > > >> actions.
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > > > Sincerely,
>> > > >> > > > > >> > > > > Dmitriy Pavlov
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > > > ср, 7 февр.
2018 г. в 13:04, Andrey Kuznetsov <
>> > > >> > > > > stkuzma@gmail.com
>> > > >> > > > > >> >:
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > > > > Hi Maxim,
>> > > >> > > > > >> > > > > >
>> > > >> > > > > >> > > > > > Regarding
your first question, the use of
>> > > >> > > afterTestsStopped
>> > > >> > > > is
>> > > >> > > > > >> not
>> > > >> > > > > >> > > > enough
>> > > >> > > > > >> > > > > > to stop
all nodes, since each individual test
>> > > >> (method)
>> > > >> > can
>> > > >> > > > > start
>> > > >> > > > > >> > > custom
>> > > >> > > > > >> > > > > set
>> > > >> > > > > >> > > > > > of notes
during its operation, and this very
>> test
>> > > >> should
>> > > >> > > > stop
>> > > >> > > > > >> all
>> > > >> > > > > >> > > those
>> > > >> > > > > >> > > > > > nodes. If
you delegate closing to
>> > afterTestsStopped
>> > > >> this
>> > > >> > > > will
>> > > >> > > > > >> > affect
>> > > >> > > > > >> > > > only
>> > > >> > > > > >> > > > > > last test
(method). Nodes that survive between
>> > > tests
>> > > >> can
>> > > >> > > > > affect
>> > > >> > > > > >> > > > > successive
>> > > >> > > > > >> > > > > > tests.
>> > > >> > > > > >> > > > > >
>> > > >> > > > > >> > > > > > 2018-02-07
1:10 GMT+03:00 Maxim Muzafarov <
>> > > >> > > > maxmuzaf@gmail.com
>> > > >> > > > > >:
>> > > >> > > > > >> > > > > >
>> > > >> > > > > >> > > > > > > Hello,
>> > > >> > > > > >> > > > > > >
>> > > >> > > > > >> > > > > > > I've
made some research about this problem
>> and
>> > i
>> > > >> think
>> > > >> > > > that
>> > > >> > > > > in
>> > > >> > > > > >> > > > general
>> > > >> > > > > >> > > > > we
>> > > >> > > > > >> > > > > > > should
move stopAllGrids method in
>> > > GridAbstractTest
>> > > >> > > class
>> > > >> > > > to
>> > > >> > > > > >> > > > > > > afterTestsStopped
method with some changes.
>> Am
>> > I
>> > > >> > right?
>> > > >> > > > > >> > > > > > >
>> > > >> > > > > >> > > > > > > Also,
I have a question about
>> > > stopAllGrids(boolean
>> > > >> > > cancel)
>> > > >> > > > > >> this
>> > > >> > > > > >> > > > > "cancel"
>> > > >> > > > > >> > > > > > > argument.
Why in some cases we should
>> interrupt
>> > > >> > > ComputeJob
>> > > >> > > > > >> and in
>> > > >> > > > > >> > > > some
>> > > >> > > > > >> > > > > > > cases
shouldn't? For example here:
>> > > >> > > > > >> > > > > > >
>> > > >> IgniteBaselineAffinityTopologyActivationTest#afterTest
>> > > >> > > > > >> > > > > > > we
call method stopAllGrids(false) this way.
>> > Why
>> > > >> not
>> > > >> > > > "true"
>> > > >> > > > > >> > > argument
>> > > >> > > > > >> > > > > > > instead?
>> > > >> > > > > >> > > > > > >
>> > > >> > > > > >> > > > > > >
>> > > >> > > > > >> > > > > > > --
>> > > >> > > > > >> > > > > > Best regards,
>> > > >> > > > > >> > > > > >   Andrey
Kuznetsov.
>> > > >> > > > > >> > > > > >
>> > > >> > > > > >> > > > >
>> > > >> > > > > >> > > >
>> > > >> > > > > >> > >
>> > > >> > > > > >> >
>> > > >> > > > > >>
>> > > >> > > > > >
>> > > >> > > > >
>> > > >> > > >
>> > > >> > >
>> > > >> >
>> > > >>
>> > > >
>> > >
>> >
>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message