ignite-dev mailing list archives

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

> Dmitry and other igniters,
>
> Will you have time to review this issue?
> I've preperated PR and TC for this, also I've fixed all comments made by
> Andrey Kuznetsov and Vyacheslav Daradur.
>
> I think this is important issue and will make test framework more stable
> and clear.
>
>
> TC: https://ci.ignite.apache.org/viewLog.html?buildId=1138151
> JIRA: https://issues.apache.org/jira/browse/IGNITE-6842
> Upsource: https://reviews.ignite.apache.org/ignite/review/IGNT-CR-502
> PR: https://github.com/apache/ignite/pull/3542
>
> чт, 15 мар. 2018 г. в 13:31, Maxim Muzafarov <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