From dev-return-31442-archive-asf-public=cust-asf.ponee.io@ignite.apache.org Tue Feb 27 15:04:54 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 4F82A180651 for ; Tue, 27 Feb 2018 15:04:53 +0100 (CET) Received: (qmail 87024 invoked by uid 500); 27 Feb 2018 14:04:52 -0000 Mailing-List: contact dev-help@ignite.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@ignite.apache.org Delivered-To: mailing list dev@ignite.apache.org Received: (qmail 87012 invoked by uid 99); 27 Feb 2018 14:04:51 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd4-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Tue, 27 Feb 2018 14:04:51 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd4-us-west.apache.org (ASF Mail Server at spamd4-us-west.apache.org) with ESMTP id 1F92AC0160 for ; Tue, 27 Feb 2018 14:04:51 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd4-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 2.379 X-Spam-Level: ** X-Spam-Status: No, score=2.379 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, KAM_NUMSUBJECT=0.5, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001] autolearn=disabled Authentication-Results: spamd4-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd4-us-west.apache.org [10.40.0.11]) (amavisd-new, port 10024) with ESMTP id bJt5fcMgVojI for ; Tue, 27 Feb 2018 14:04:46 +0000 (UTC) Received: from mail-qt0-f170.google.com (mail-qt0-f170.google.com [209.85.216.170]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id ECA1A5F254 for ; Tue, 27 Feb 2018 14:04:45 +0000 (UTC) Received: by mail-qt0-f170.google.com with SMTP id r16so14307593qtm.4 for ; Tue, 27 Feb 2018 06:04:45 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to; bh=RoqtxveVRUFHMfTlTLGM25s0OxVB7zUb7q8XyQlrqNQ=; b=g54zskNulMme7L5UAoI1AqxPQvYQ3Lg+tu3vLzGZqXKy3oQX7YSaYhrzzaV+3267Eu QVipJaCv4vRINJSAwEcTrSnwyazf38zTW0zJ4fdbYGKKIyhP5oWdEFoRyfcdnd5OGcu/ ATKl7F9fk8JN0pjTJUUl3UMDyiN89oBAY7Sc+OKloY0tpu46VAd4hkbmwBXiYPZNTOkB uZAscJEsIj9Roo+je6H344EtzvR/YIO9qNGHc3XFpg2YEszxaYP1RhqNB1HlYaqK5BDW e4PDxKQ1LXES2w8nXf44BdjXvC9DWCxZoIPmp5RDxzSwh1lIC9A2Toh/a1wHwDTwwtjZ sMnw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to; bh=RoqtxveVRUFHMfTlTLGM25s0OxVB7zUb7q8XyQlrqNQ=; b=HWBcb2RXH2iOPXAsy6Or+HEDoJpKN/q6O/UKPHhBLI7q8MQXUJsWlR8uiWyALPDu0B oeCa8eeSbPnWQqwf154qBJPK1Yc/GD6/s06cRFlEeeAH5r2q4r11GasRmB1l0roEpBCC wfF8luSEDjmNRXAVK0bx6tMBjADLJvTmPdieTX4smHluS69thHp/FijfG1wreCvwgRru nrhXiPLhCiHWBvpDzE1wzjACmsSuaxwoV7bTSLRIROiqTE4cLFQwybXKe/lXrfjwPgm7 gFsBxssCabIOIVYamupzrwKRndueBo8PC+DmZxUBX8DV8btekKM1d/Lg8vaASLyVF7EK 7UvQ== X-Gm-Message-State: APf1xPC51s+sBCGo1EuyXsTm6CI5711xWSEiMMgr7ejgJ6OEtldF7WTz BYzkYuMQj7yserw/WFgEGH+QUN3rt6YnpqR/c3Y= X-Google-Smtp-Source: AG47ELtK4re6rV8OHuT4P/m3yxFPWgT41kuTPCdfAoCQgTojeLPkAGjcAIv12HXSxajUjqnpQRYymq9f9+vh7RWKP1M= X-Received: by 10.200.57.97 with SMTP id t30mr24121198qtb.22.1519740284566; Tue, 27 Feb 2018 06:04:44 -0800 (PST) MIME-Version: 1.0 References: <0B392DA6-09A6-4EDC-B1EE-4F50851BCBBE@apache.org> In-Reply-To: From: Maxim Muzafarov Date: Tue, 27 Feb 2018 14:04:33 +0000 Message-ID: Subject: Re: Stop nodes after test by default - IGNITE-6842 To: dev@ignite.apache.org Content-Type: multipart/alternative; boundary="001a113e6bfcd0023a0566321bcd" --001a113e6bfcd0023a0566321bcd Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Yep, link may not be correct. Here is correct version: TC: *https://ci.ignite.apache.org/project.html?projectId=3DIgniteTests24Jav= a8&branch_IgniteTests24Java8=3Dpull%2F3542%2Fhead * =D0=B2=D1=82, 27 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0=B2 16:41, Dmitr= y Pavlov : > Hi Maxim, > > could you please provide link to TC run on your PR? It seems link provide= d > 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 > > =D0=B2=D1=82, 27 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0=B2 16:14, Max= im Muzafarov : > > > 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=3D723895&persona= l=3Dfalse&buildTypeId=3D&tab=3DvcsModificationTests > > JIRA: https://issues.apache.org/jira/browse/IGNITE-6842 > > > > > > =D1=87=D1=82, 22 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0=B2 14:12, D= mitry 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 > > > > > > =D1=87=D1=82, 22 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0=B2 13:44,= Maxim Muzafarov : > > > > > > > Hi all, > > > > > > > > I've made some changes based on our previous discusstions, please s= ee > > 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 no= t > > > > started yet. Am I right? > > > > 2) I think we should call stopAllGrids in tearDown method. So, if i= n > > some > > > > cases we'll override afterTestsStop in subclasses and forgot to cal= l > > > > 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=3D7172= 75 > > > > [3] https://issues.apache.org/jira/browse/IGNITE-6842 > > > > > > > > > > > > =D1=81=D1=80, 7 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0=B2 18:28= , Maxim Muzafarov : > > > > > > > > > Thank you all, > > > > > > > > > > I'll add this comment's for JIRA ticket, if you don't mind. > > > > > > > > > > =D1=81=D1=80, 7 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0=B2 15:= 16, Dmitry 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 ea= ch > > > test. > > > > >> > > > > >> =D1=81=D1=80, 7 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0=B2 15= :13, Anton Vinogradov < > > > avinogradov@gridgain.com > > > > >: > > > > >> > > > > >> > Maxim, > > > > >> > > > > > >> > We discussed with Dima privately, and decided > > > > >> > > > > > >> > 1) We have to assert that there is no alive nodes at > > > > GridAbstractTest's > > > > >> > beforeTestsStarted > > > > >> > 2) We have to kill all alive nodes (without force) at > > > > GridAbstractTest's > > > > >> > afterTestsStopped > > > > >> > 3) In case of any exceptions at #2 we have to see test error > > > > >> > 4) We can get rid of all useless stopAllGrids at > > GridAbstractTest's > > > > >> > subclasses. > > > > >> > > > > > >> > > > > > >> > > > > > >> > On Wed, Feb 7, 2018 at 2:32 PM, Dmitry Pavlov < > > > dpavlov.spb@gmail.com> > > > > >> > wrote: > > > > >> > > > > > >> > > > Let's just add stopAllGrids(flase) to GridAbstractTest 's > > > > >> > > afterTestsStopped > > > > >> > > method body. > > > > >> > > > > > > >> > > Can't agree with it becase implicit silent shutdown of nodes > > from > > > > test > > > > >> > > framework may cause a lot of bugs introduced to Ignite. > > > > >> > > > > > > >> > > I think stop+test fail should occur. > > > > >> > > In that case author of incorrect test or not consistent Igni= te > > > > >> shutdown > > > > >> > > will see problem. > > > > >> > > > > > > >> > > 'Fail fast' if always better than hidding real problem under > > > > automatic > > > > >> > > framework feature. > > > > >> > > > > > > >> > > =D1=81=D1=80, 7 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. =D0= =B2 14:05, Anton Vinogradov < > > > > >> avinogradov@gridgain.com > > > > >> > >: > > > > >> > > > > > > >> > > > Hi all, > > > > >> > > > > > > > >> > > > > I've made some research about this problem and i think > that > > in > > > > >> > general > > > > >> > > we > > > > >> > > > > should move stopAllGrids method in GridAbstractTest clas= s > 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) thi= s > > > > "cancel" > > > > >> > > > That's just a flag means "do not wait for any operations > > finish" > > > > >> > > > See no reason to set it true in that case. > > > > >> > > > > > > > >> > > > > If you delegate closing to afterTestsStopped this will > > affect > > > > only > > > > >> > > > > last test (method). > > > > >> > > > The idea is to stop all nodes at last test's method finish= . > > > > >> > > > > > > > >> > > > > Nodes that survive between tests can affect successive > > > > >> > > > tests. > > > > >> > > > Thats ok. we have a lot tests where nodes reusable between > > > test's > > > > >> > > methods. > > > > >> > > > > > > > >> > > > > > > > >> > > > On Wed, Feb 7, 2018 at 1:20 PM, Dmitry Pavlov < > > > > >> dpavlov.spb@gmail.com> > > > > >> > > > wrote: > > > > >> > > > > > > > >> > > > > Hi Igniters, > > > > >> > > > > > > > > >> > > > > IMO nodes that survive between tests is not general > practice > > > and > > > > >> I'm > > > > >> > > not > > > > >> > > > > sure is a best practice. I suggest to mark such tests wi= th > > > some > > > > >> > method > > > > >> > > > > overriden from AbstractTest. > > > > >> > > > > > > > > >> > > > > About cancel flag, please note stopAllGrids(boolean > cancel) > > > > >> > > cancel=3Dfalse > > > > >> > > > > 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 > > > > >> > > > > > > > > >> > > > > =D1=81=D1=80, 7 =D1=84=D0=B5=D0=B2=D1=80. 2018 =D0=B3. = =D0=B2 13:04, Andrey Kuznetsov < > > > > stkuzma@gmail.com > > > > >> >: > > > > >> > > > > > > > > >> > > > > > Hi Maxim, > > > > >> > > > > > > > > > >> > > > > > Regarding your first question, the use of > > afterTestsStopped > > > is > > > > >> not > > > > >> > > > enough > > > > >> > > > > > to stop all nodes, since each individual test (method) > can > > > > start > > > > >> > > custom > > > > >> > > > > set > > > > >> > > > > > of notes during its operation, and this very test shou= ld > > > stop > > > > >> all > > > > >> > > those > > > > >> > > > > > nodes. If you delegate closing to afterTestsStopped th= is > > > will > > > > >> > affect > > > > >> > > > only > > > > >> > > > > > last test (method). Nodes that survive between tests c= an > > > > affect > > > > >> > > > > successive > > > > >> > > > > > tests. > > > > >> > > > > > > > > > >> > > > > > 2018-02-07 1:10 GMT+03:00 Maxim Muzafarov < > > > maxmuzaf@gmail.com > > > > >: > > > > >> > > > > > > > > > >> > > > > > > Hello, > > > > >> > > > > > > > > > > >> > > > > > > I've made some research about this problem and i thi= nk > > > 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#afterTe= st > > > > >> > > > > > > we call method stopAllGrids(false) this way. Why not > > > "true" > > > > >> > > argument > > > > >> > > > > > > instead? > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > -- > > > > >> > > > > > Best regards, > > > > >> > > > > > Andrey Kuznetsov. > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > > --001a113e6bfcd0023a0566321bcd--