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 Thu, 22 Feb 2018 11:12:07 GMT
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