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 Thu, 22 Feb 2018 10:43:50 GMT
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