Stop nodes after test by default - IGNITE-6842

[Mmuzaf]

Hello everyone!

I would like to take this one issue for implicating myself into Ignite
community.

https://issues.apache.org/jira/browse/IGNITE-6842

Can you help me with granting contributor permissons?
My JIRA ID: mmuzaf
e-mail: [hidden email]
So I can assign this ticket for myself, If you don't mind of course.

Also, it will be great to have something like general roadmap for fixing
this issue. What should I look at on my first step?

[dmagda]

Hello Maxim,

Granted you all the required permissions in JIRA. Feel free to assign the ticket to your account.

In general, review how the testing framework works in Ignite and you’ll get why we stop all the nodes manually. It will help to get answers on most of the questions. Please propose your solution or ask for advice here after that.

—
Denis

> On Jan 31, 2018, at 8:01 AM, Maxim Muzafarov <[hidden email]> wrote:
>
> Hello everyone!
>
> I would like to take this one issue for implicating myself into Ignite
> community.
>
> https://issues.apache.org/jira/browse/IGNITE-6842
>
> Can you help me with granting contributor permissons?
> My JIRA ID: mmuzaf
> e-mail: [hidden email]
> So I can assign this ticket for myself, If you don't mind of course.
>
> Also, it will be great to have something like general roadmap for fixing
> this issue. What should I look at on my first step?

[Dmitriy Pavlov]

Hi Maxim,

I would be happy if this issue is completed.

I think best solultion is
1) to validate remained up and running nodes
2) and fail test if not-stopped nodes remained

instead of silently stop node. If nodes will be stopped silently, probable
errors may be missed both in test and in Ignite code.

Sincerely,
Dmitriy Pavlov

чт, 1 февр. 2018 г. в 3:54, Denis Magda <[hidden email]>:

> Hello Maxim,
>
> Granted you all the required permissions in JIRA. Feel free to assign the
> ticket to your account.
>
> In general, review how the testing framework works in Ignite and you’ll
> get why we stop all the nodes manually. It will help to get answers on most
> of the questions. Please propose your solution or ask for advice here after
> that.
>
> —
> Denis
>
> > On Jan 31, 2018, at 8:01 AM, Maxim Muzafarov <[hidden email]> wrote:
> >
> > Hello everyone!
> >
> > I would like to take this one issue for implicating myself into Ignite
> > community.
> >
> > https://issues.apache.org/jira/browse/IGNITE-6842
> >
> > Can you help me with granting contributor permissons?
> > My JIRA ID: mmuzaf
> > e-mail: [hidden email]
> > So I can assign this ticket for myself, If you don't mind of course.
> >
> > Also, it will be great to have something like general roadmap for fixing
> > this issue. What should I look at on my first step?
>
>

[Mmuzaf]

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?



чт, 1 февр. 2018 г. в 16:12, Dmitry Pavlov <[hidden email]>:

> Hi Maxim,
>
> I would be happy if this issue is completed.
>
> I think best solultion is
> 1) to validate remained up and running nodes
> 2) and fail test if not-stopped nodes remained
>
> instead of silently stop node. If nodes will be stopped silently, probable
> errors may be missed both in test and in Ignite code.
>
> Sincerely,
> Dmitriy Pavlov
>
> чт, 1 февр. 2018 г. в 3:54, Denis Magda <[hidden email]>:
>
> > Hello Maxim,
> >
> > Granted you all the required permissions in JIRA. Feel free to assign the
> > ticket to your account.
> >
> > In general, review how the testing framework works in Ignite and you’ll
> > get why we stop all the nodes manually. It will help to get answers on
> most
> > of the questions. Please propose your solution or ask for advice here
> after
> > that.
> >
> > —
> > Denis
> >
> > > On Jan 31, 2018, at 8:01 AM, Maxim Muzafarov <[hidden email]>
> wrote:
> > >
> > > Hello everyone!
> > >
> > > I would like to take this one issue for implicating myself into Ignite
> > > community.
> > >
> > > https://issues.apache.org/jira/browse/IGNITE-6842
> > >
> > > Can you help me with granting contributor permissons?
> > > My JIRA ID: mmuzaf
> > > e-mail: [hidden email]
> > > So I can assign this ticket for myself, If you don't mind of course.
> > >
> > > Also, it will be great to have something like general roadmap for
> fixing
> > > this issue. What should I look at on my first step?
> >
> >
>

[Andrey Kuznetsov]

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 <[hidden email]>:

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

[Dmitriy Pavlov]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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.
>

[Anton Vinogradov]

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 <[hidden email]> 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 <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > 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.
> >
>

[Dmitriy Pavlov]

> 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 <[hidden email]>:

> 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 <[hidden email]>
> 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 <[hidden email]>:
> >
> > > 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 <[hidden email]>:
> > >
> > > > 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.
> > >
> >
>

[Anton Vinogradov]

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 <[hidden email]> 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 <[hidden email]>:
>
> > 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 <[hidden email]>
> > 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 <[hidden email]>:
> > >
> > > > 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 <[hidden email]>:
> > > >
> > > > > 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.
> > > >
> > >
> >
>

[Dmitriy Pavlov]

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 <[hidden email]>:

> 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 <[hidden email]>
> 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 <[hidden email]
> >:
> >
> > > 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 <[hidden email]>
> > > 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 <[hidden email]>:
> > > >
> > > > > 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 <[hidden email]>:
> > > > >
> > > > > > 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.
> > > > >
> > > >
> > >
> >
>

[Mmuzaf]

Thank you all,

I'll add this comment's for JIRA ticket, if you don't mind.

ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <[hidden email]>
> > 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 <
> [hidden email]
> > >:
> > >
> > > > 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 <[hidden email]
> >
> > > > 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 <[hidden email]>:
> > > > >
> > > > > > 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 <[hidden email]>:
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > >
> > > >
> > >
> >
>

[Mmuzaf]

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 <[hidden email]>:

> Thank you all,
>
> I'll add this comment's for JIRA ticket, if you don't mind.
>
> ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <[hidden email]>:
>
>> 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 <[hidden email]>:
>>
>> > 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 <[hidden email]>
>> > 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 <
>> [hidden email]
>> > >:
>> > >
>> > > > 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 <
>> [hidden email]>
>> > > > 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 <[hidden email]
>> >:
>> > > > >
>> > > > > > 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 <[hidden email]>:
>> > > > > >
>> > > > > > > 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.
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>

[Dmitriy Pavlov]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > Thank you all,
> >
> > I'll add this comment's for JIRA ticket, if you don't mind.
> >
> > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <[hidden email]>:
> >
> >> 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 <[hidden email]
> >:
> >>
> >> > 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 <[hidden email]>
> >> > 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 <
> >> [hidden email]
> >> > >:
> >> > >
> >> > > > 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 <
> >> [hidden email]>
> >> > > > 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 <
> [hidden email]
> >> >:
> >> > > > >
> >> > > > > > 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 <[hidden email]
> >:
> >> > > > > >
> >> > > > > > > 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.
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
>

[Mmuzaf]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > Thank you all,
> > >
> > > I'll add this comment's for JIRA ticket, if you don't mind.
> > >
> > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <[hidden email]>:
> > >
> > >> 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 <
> [hidden email]
> > >:
> > >>
> > >> > 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 <
> [hidden email]>
> > >> > 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 <
> > >> [hidden email]
> > >> > >:
> > >> > >
> > >> > > > 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 <
> > >> [hidden email]>
> > >> > > > 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 <
> > [hidden email]
> > >> >:
> > >> > > > >
> > >> > > > > > 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 <
> [hidden email]
> > >:
> > >> > > > > >
> > >> > > > > > > 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.
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>

[Dmitriy Pavlov]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > 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 <[hidden email]>:
> > >
> > > > Thank you all,
> > > >
> > > > I'll add this comment's for JIRA ticket, if you don't mind.
> > > >
> > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <[hidden email]>:
> > > >
> > > >> 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 <
> > [hidden email]
> > > >:
> > > >>
> > > >> > 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 <
> > [hidden email]>
> > > >> > 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 <
> > > >> [hidden email]
> > > >> > >:
> > > >> > >
> > > >> > > > 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 <
> > > >> [hidden email]>
> > > >> > > > 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 <
> > > [hidden email]
> > > >> >:
> > > >> > > > >
> > > >> > > > > > 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 <
> > [hidden email]
> > > >:
> > > >> > > > > >
> > > >> > > > > > > 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.
> > > >> > > > > >
> > > >> > > > >
> > > >> > > >
> > > >> > >
> > > >> >
> > > >>
> > > >
> > >
> >
>

[Mmuzaf]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > 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 <[hidden email]>:
> > >
> > > > 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 <[hidden email]>:
> > > >
> > > > > Thank you all,
> > > > >
> > > > > I'll add this comment's for JIRA ticket, if you don't mind.
> > > > >
> > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <[hidden email]
> >:
> > > > >
> > > > >> 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 <
> > > [hidden email]
> > > > >:
> > > > >>
> > > > >> > 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 <
> > > [hidden email]>
> > > > >> > 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 <
> > > > >> [hidden email]
> > > > >> > >:
> > > > >> > >
> > > > >> > > > 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 <
> > > > >> [hidden email]>
> > > > >> > > > 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 <
> > > > [hidden email]
> > > > >> >:
> > > > >> > > > >
> > > > >> > > > > > 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 <
> > > [hidden email]
> > > > >:
> > > > >> > > > > >
> > > > >> > > > > > > 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.
> > > > >> > > > > >
> > > > >> > > > >
> > > > >> > > >
> > > > >> > >
> > > > >> >
> > > > >>
> > > > >
> > > >
> > >
> >
>

[Dmitriy Pavlov]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > 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 <[hidden email]>:
> > >
> > > > 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 <[hidden email]>:
> > > >
> > > > > 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 <[hidden email]>:
> > > > >
> > > > > > Thank you all,
> > > > > >
> > > > > > I'll add this comment's for JIRA ticket, if you don't mind.
> > > > > >
> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
> [hidden email]
> > >:
> > > > > >
> > > > > >> 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 <
> > > > [hidden email]
> > > > > >:
> > > > > >>
> > > > > >> > 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 <
> > > > [hidden email]>
> > > > > >> > 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 <
> > > > > >> [hidden email]
> > > > > >> > >:
> > > > > >> > >
> > > > > >> > > > 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 <
> > > > > >> [hidden email]>
> > > > > >> > > > 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 <
> > > > > [hidden email]
> > > > > >> >:
> > > > > >> > > > >
> > > > > >> > > > > > 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 <
> > > > [hidden email]
> > > > > >:
> > > > > >> > > > > >
> > > > > >> > > > > > > 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.
> > > > > >> > > > > >
> > > > > >> > > > >
> > > > > >> > > >
> > > > > >> > >
> > > > > >> >
> > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> >
>

[Dmitriy Pavlov]

Hi Maxim,

are there any news on these test fails?

Is issue ready for review?

Sincerely,
Dmitiry Pavlov

вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]>:

> 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 <[hidden email]>:
>
>> 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 <[hidden email]>:
>>
>> > 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 <[hidden email]>:
>> >
>> > > 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 <[hidden email]>:
>> > >
>> > > > 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 <[hidden email]>:
>> > > >
>> > > > > 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 <[hidden email]
>> >:
>> > > > >
>> > > > > > Thank you all,
>> > > > > >
>> > > > > > I'll add this comment's for JIRA ticket, if you don't mind.
>> > > > > >
>> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
>> [hidden email]
>> > >:
>> > > > > >
>> > > > > >> 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 <
>> > > > [hidden email]
>> > > > > >:
>> > > > > >>
>> > > > > >> > 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 <
>> > > > [hidden email]>
>> > > > > >> > 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 <
>> > > > > >> [hidden email]
>> > > > > >> > >:
>> > > > > >> > >
>> > > > > >> > > > 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 <
>> > > > > >> [hidden email]>
>> > > > > >> > > > 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 <
>> > > > > [hidden email]
>> > > > > >> >:
>> > > > > >> > > > >
>> > > > > >> > > > > > 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 <
>> > > > [hidden email]
>> > > > > >:
>> > > > > >> > > > > >
>> > > > > >> > > > > > > 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.
>> > > > > >> > > > > >
>> > > > > >> > > > >
>> > > > > >> > > >
>> > > > > >> > >
>> > > > > >> >
>> > > > > >>
>> > > > > >
>> > > > >
>> > > >
>> > >
>> >
>>
>

[Mmuzaf]

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 <[hidden email]>:

> Hi Maxim,
>
> are there any news on these test fails?
>
> Is issue ready for review?
>
> Sincerely,
> Dmitiry Pavlov
>
> вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> >> 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 <[hidden email]>:
> >>
> >> > 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 <[hidden email]>:
> >> >
> >> > > 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 <[hidden email]
> >:
> >> > >
> >> > > > 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 <[hidden email]
> >:
> >> > > >
> >> > > > > 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 <
> [hidden email]
> >> >:
> >> > > > >
> >> > > > > > Thank you all,
> >> > > > > >
> >> > > > > > I'll add this comment's for JIRA ticket, if you don't mind.
> >> > > > > >
> >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
> >> [hidden email]
> >> > >:
> >> > > > > >
> >> > > > > >> 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 <
> >> > > > [hidden email]
> >> > > > > >:
> >> > > > > >>
> >> > > > > >> > 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 <
> >> > > > [hidden email]>
> >> > > > > >> > 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 <
> >> > > > > >> [hidden email]
> >> > > > > >> > >:
> >> > > > > >> > >
> >> > > > > >> > > > 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 <
> >> > > > > >> [hidden email]>
> >> > > > > >> > > > 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 <
> >> > > > > [hidden email]
> >> > > > > >> >:
> >> > > > > >> > > > >
> >> > > > > >> > > > > > 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 <
> >> > > > [hidden email]
> >> > > > > >:
> >> > > > > >> > > > > >
> >> > > > > >> > > > > > > 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.
> >> > > > > >> > > > > >
> >> > > > > >> > > > >
> >> > > > > >> > > >
> >> > > > > >> > >
> >> > > > > >> >
> >> > > > > >>
> >> > > > > >
> >> > > > >
> >> > > >
> >> > >
> >> >
> >>
> >
>

[Dmitriy Pavlov]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > Hi Maxim,
> >
> > are there any news on these test fails?
> >
> > Is issue ready for review?
> >
> > Sincerely,
> > Dmitiry Pavlov
> >
> > вт, 27 февр. 2018 г. в 17:12, Dmitry Pavlov <[hidden email]>:
> >
> > > 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 <[hidden email]>:
> > >
> > >> 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 <[hidden email]>:
> > >>
> > >> > 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 <[hidden email]>:
> > >> >
> > >> > > 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 <
> [hidden email]
> > >:
> > >> > >
> > >> > > > 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 <
> [hidden email]
> > >:
> > >> > > >
> > >> > > > > 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 <
> > [hidden email]
> > >> >:
> > >> > > > >
> > >> > > > > > Thank you all,
> > >> > > > > >
> > >> > > > > > I'll add this comment's for JIRA ticket, if you don't mind.
> > >> > > > > >
> > >> > > > > > ср, 7 февр. 2018 г. в 15:16, Dmitry Pavlov <
> > >> [hidden email]
> > >> > >:
> > >> > > > > >
> > >> > > > > >> 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 <
> > >> > > > [hidden email]
> > >> > > > > >:
> > >> > > > > >>
> > >> > > > > >> > 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 <
> > >> > > > [hidden email]>
> > >> > > > > >> > 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 <
> > >> > > > > >> [hidden email]
> > >> > > > > >> > >:
> > >> > > > > >> > >
> > >> > > > > >> > > > 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 <
> > >> > > > > >> [hidden email]>
> > >> > > > > >> > > > 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 <
> > >> > > > > [hidden email]
> > >> > > > > >> >:
> > >> > > > > >> > > > >
> > >> > > > > >> > > > > > 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 <
> > >> > > > [hidden email]
> > >> > > > > >:
> > >> > > > > >> > > > > >
> > >> > > > > >> > > > > > > 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.
> > >> > > > > >> > > > > >
> > >> > > > > >> > > > >
> > >> > > > > >> > > >
> > >> > > > > >> > >
> > >> > > > > >> >
> > >> > > > > >>
> > >> > > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>