ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Govorukhin <dmitriy.govoruk...@gmail.com>
Subject Re: Stop nodes after test by default - IGNITE-6842
Date Tue, 20 Mar 2018 08:49:15 GMT
Looks good for me, please merge.

19 мар. 2018 г. 3:22 ПП пользователь "Dmitry Pavlov" <dpavlov.spb@gmail.com>
написал:

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