ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ipavlukhin <vololo...@gmail.com>
Subject Re: Ticket review checklist
Date Tue, 14 Aug 2018 16:16:41 GMT
Hi Igniters,

I would like to refresh review checklist a little bit. Currently it [1] 
contains section against lambda Lambda expressions and Stream API. As 
far as I know it is not true anymore. Currently both features have 
theirs usage in core module. What is a state of affairs for a subject? 
Are there some well-known cases where e.g. lambdas are not applicable? 
Should we document it?

I personally think that we could delete entire Java 8 section from 
checklist (and Java 5 as well). I understand that every tool should be 
used judiciously but I doubt that all cases can be covered in short 
checklist.

[1] 
https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Java8


On 2018/07/09 20:53:42, Dmitry Pavlov <d...@gmail.com> wrote:
 > I also tend to agree about updating checklist>
 >
 > About suite timeouts, I suspect there is one problem introduced 
recently>
 > within 3 days, which caused this mass timeouts.>
 >
 > I hope Igniters will find out reason soon. In relation to compute we 
have>
 > only 2 possible cause:>
 > Ivan Daschinskiy (idaschinskiy) 2 files IGNITE-8869 Fixed>
 > PartitionsExchangeOnDiscoveryHistoryOverflowTest hanging>
 > Signed-off-by: Andrey Gura <ag...@apache.org> ···>
 >
 > Dmitriy Govorukhin (dgovorukhin) 12 files IGNITE-8827 Disable WAL 
during>
 > apply updates on recovery>
 >
 > I guess if we fix this reason we will fix 10 suites more>
 > References:>
 > 
https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ComputeGrid&tab=buildTypeHistoryList&branch_IgniteTests24Java8=%3Cdefault%3E>


 >
 >
 > пн, 9 июл. 2018 г. в 22:29, Anton Vinogradov <av...@apache.org>:>
 >
 > > Sounds reasonable.>
 > > I've satrted Data Structures suite hang investigation [1].>
 > >>
 > > Igniters, especially commiters,>
 > > I know, you're busy, but it will be a great help to the project in 
case you>
 > > fix at least one hang per person.>
 > >>
 > > [1] https://issues.apache.org/jira/browse/IGNITE-8783>
 > >>
 > > пн, 9 июл. 2018 г. в 19:24, Maxim Muzafarov <ma...@gmail.com>:>
 > >>
 > > > Hi Igniters,>
 > > >>
 > > > Let's back to discussion of review checklist. Can we add more>
 > > clarification>
 > > > about running all suites on TeamCity?>
 > > >>
 > > > My suggestion is: “All test suites MUST be run on TeamCity [3] 
before>
 > > merge>
 > > > to master, there MUST NOT be any test failures * and any 
tests\suites>
 > > with>
 > > > “execution timeouts” *. Not important test failures should be 
muted and>
 > > > handled according to [4] process.”>
 > > >>
 > > > As you can see we have stable “Execution timeouts” for>
 > > > “Activate\Deactiveate Cluster” test suite since 16-th June. How 
can we be>
 > > > sure in this case that new changes would not break up old 
functionality?>
 > > >>
 > > > From my point, all new changes MUST NOT be merged to master util 
we will>
 > > > fix all execution timeouts for suites. Even if current changes 
are not>
 > > > related to these timeouts.>
 > > >>
 > > > [1]>
 > > >>
 > > >>
 > > 
https://ci.ignite.apache.org/viewType.html?buildTypeId=IgniteTests24Java8_ActivateDeactivateCluster&tab=buildTypeHistoryList&branch_IgniteTests24Java8=%3Cdefault%3E>


 > > >>
 > > >>
 > > > пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov <dp...@gmail.com>:>
 > > >>
 > > > > Requirement of green TC for each PR is community rule, not my.>
 > > > >>
 > > > > I'll answer ro another question, what should we do with test 
failure:>
 > > > > "Ideally - fix, but at least mute test and create ticket. ">
 > > > >>
 > > > > May be it's time to formalize Make TC Green Again process in 
details,>
 > > so>
 > > > > let me share my draft.>
 > > > >>
 > > > > If Igniter see test failure (in master, in release bracnh, 
etc), he>
 > > > should>
 > > > > consider following steps:>
 > > > >>
 > > > > - If your changes can led to this failure(s), please create issue>
 > > with>
 > > > > label MakeTeamCityGreenAgain and assign it to you.>
 > > > > - If you have fix, please set ticket to PA state and write to dev>
 > > > > list fix is ready.>
 > > > > - For case fix will require some time please mute test and set>
 > > > label>
 > > > > Muted_Test to issue>
 > > > > - If you know which change caused failure please contact change>
 > > author>
 > > > > directly.>
 > > > > - If you don't know which change caused failure please send 
message>
 > > to>
 > > > > dev list to find out>
 > > > >>
 > > > >>
 > > > >>
 > > > >>
 > > > > пн, 4 июн. 2018 г. в 15:27, Vladimir Ozerov <vo...@gridgain.com>:>
 > > > >>
 > > > > > Dmitry,>
 > > > > >>
 > > > > > My question was how to proceed with your rules. Could you 
please>
 > > > clarify?>
 > > > > >>
 > > > > > On Mon, Jun 4, 2018 at 2:52 PM, Dmitry Pavlov 
<dpavlov.spb@gmail.com>
 > > >>
 > > > > > wrote:>
 > > > > >>
 > > > > > > Vladimir, I mean strict definition, how much previous runs

should>
 > > > > > > contributor consider? What if test was failed by 
infrastructure>
 > > > reason>
 > > > > in>
 > > > > > > master previously, how can contributor be sure test failure

!=>
 > > broken>
 > > > > > code>
 > > > > > > in PR? In this case it should be double checked by>
 > > > > contributor/reviewer.>
 > > > > > > I'm sure nobody can give strict definition of 'new' failure.>
 > > > > > >>
 > > > > > > Flaky tests detected by TC may be taken into account in 
check-list,>
 > > > > > because>
 > > > > > > contributor can check if failure is flaky. But again, not 
all tests>
 > > > > with>
 > > > > > > floating failure is detected by TC as flaky.>
 > > > > > >>
 > > > > > > I don't understand what problem will be solved if we soften

current>
 > > > > > > requirement with 'new' test? Everybody will continue to 
complain>
 > > they>
 > > > > > PR's>
 > > > > > > test failures is not `new`. So let's keep it as is.>
 > > > > > >>
 > > > > > > пн, 4 июн. 2018 г. в 14:46, Vladimir Ozerov 
<vozerov@gridgain.com>
 > > >:>
 > > > > > >>
 > > > > > > > Dmitry,>
 > > > > > > >>
 > > > > > > > New failure is a failure hasn't happened on previous 
runs. If it>
 > > do>
 > > > > > > > happened, then contributor should see if it is a flaky
or 
not>
 > > > through>
 > > > > > > local>
 > > > > > > > and TC runs. The same works for timeout suites.>
 > > > > > > > Current statement in "Review Checklist" that there are

should be>
 > > no>
 > > > > > > failed>
 > > > > > > > tests is not applicable to real word. Almost every patch
is>
 > > pushed>
 > > > to>
 > > > > > > > repository with test failures.>
 > > > > > > >>
 > > > > > > > On Mon, Jun 4, 2018 at 2:22 PM, Dmitry Pavlov <>
 > > > dpavlov.spb@gmail.com>
 > > > > >>
 > > > > > > > wrote:>
 > > > > > > >>
 > > > > > > > > Hi Vladimir, could you provide definition what is
new 
failure?>
 > > > how>
 > > > > do>
 > > > > > > you>
 > > > > > > > > know it is new or not?>
 > > > > > > > >>
 > > > > > > > > And please forget for a moment you're Ignite expert
& 
veteran,>
 > > > > > imagine>
 > > > > > > > you>
 > > > > > > > > are newcomer.>
 > > > > > > > >>
 > > > > > > > > I can't find any criteria that can be used by newbie
to 
come to>
 > > > the>
 > > > > > > > > conclusion that test is new. Patch is accepted by

reviewer, so>
 > > it>
 > > > > > > should>
 > > > > > > > be>
 > > > > > > > > up to him to correctly register failures in tickets
with>
 > > > > > > > > MakeTeamCityGreenAgain label and mute unimportant
tests.>
 > > > > > > > >>
 > > > > > > > > пн, 4 июн. 2018 г. в 11:32, Vladimir Ozerov
<>
 > > > vozerov@gridgain.com>
 > > > > >:>
 > > > > > > > >>
 > > > > > > > > > Dmitry,>
 > > > > > > > > >>
 > > > > > > > > > I still do not see how new patches could be
accepted 
with>
 > > this>
 > > > > > > > > requirement>
 > > > > > > > > > in place. Consider the following case: I created
a 
patch and>
 > > > run>
 > > > > it>
 > > > > > > on>
 > > > > > > > > TC,>
 > > > > > > > > > observed N failures, verified through TC history
that 
none if>
 > > > > them>
 > > > > > > are>
 > > > > > > > > new.>
 > > > > > > > > > Am I eligible to push the commit?>
 > > > > > > > > >>
 > > > > > > > > > On Thu, May 24, 2018 at 3:11 PM, Dmitry Pavlov
<>
 > > > > > > dpavlov.spb@gmail.com>>
 > > > > > > > > > wrote:>
 > > > > > > > > >>
 > > > > > > > > > > Petr, good point. It is more intuitive,
we should 
mark test>
 > > > we>
 > > > > > can>
 > > > > > > > > ignore>
 > > > > > > > > > > by mute.>
 > > > > > > > > > >>
 > > > > > > > > > > So Vladimir, you or other Ignite veteran
can mute 
test, if>
 > > > can>
 > > > > > say>
 > > > > > > it>
 > > > > > > > > is>
 > > > > > > > > > > not important.>
 > > > > > > > > > >>
 > > > > > > > > > > чт, 24 мая 2018 г. в 15:07, Petr
Ivanov <>
 > > mr.weider@gmail.com>
 > > > >:>
 > > > > > > > > > >>
 > > > > > > > > > > > Why cannot we mute (and file corresponding

tickets) all>
 > > > test>
 > > > > > > > failures>
 > > > > > > > > > > > (including flaky) to some date and
start 
initiative Green>
 > > > TC?>
 > > > > > > > > > > >>
 > > > > > > > > > > >>
 > > > > > > > > > > >>
 > > > > > > > > > > > > On 24 May 2018, at 15:04, Vladimir
Ozerov <>
 > > > > > > vozerov@gridgain.com>>
 > > > > > > > > > > wrote:>
 > > > > > > > > > > > >>
 > > > > > > > > > > > > Dmitry,>
 > > > > > > > > > > > >>
 > > > > > > > > > > > > We cannot add this requirements,
because we do 
have>
 > > > > failures>
 > > > > > on>
 > > > > > > > TC.>
 > > > > > > > > > > This>
 > > > > > > > > > > > > requirement implies that all
development would 
stop>
 > > until>
 > > > > TC>
 > > > > > is>
 > > > > > > > > > green.>
 > > > > > > > > > > > > We never had old requirement
work, neither we 
need to>
 > > > > enforce>
 > > > > > > it>
 > > > > > > > > now.>
 > > > > > > > > > > > >>
 > > > > > > > > > > > > On Thu, May 24, 2018 at 2:59
PM, Dmitry Pavlov <>
 > > > > > > > > > dpavlov.spb@gmail.com>>
 > > > > > > > > > > > > wrote:>
 > > > > > > > > > > > >>
 > > > > > > > > > > > >> 3.c>
 > > > > > > > > > > > >>>
 > > > > > > > > > > > >> 1. All test suites *MUST*
be run on TeamCity [3]>
 > > > before>
 > > > > > > merge>
 > > > > > > > to>
 > > > > > > > > > > > master,>
 > > > > > > > > > > > >> there *MUST NOT* be any test
failures>
 > > > > > > > > > > > >>>
 > > > > > > > > > > > >>>
 > > > > > > > > > > > >> 'New' word should be removed
because we cant 
separate>
 > > > > `new`>
 > > > > > > and>
 > > > > > > > > `non>
 > > > > > > > > > > > new`>
 > > > > > > > > > > > >> failures.>
 > > > > > > > > > > > >>>
 > > > > > > > > > > > >> Let's imagine example, we
have 50 green runs in>
 > > master.>
 > > > > And>
 > > > > > PR>
 > > > > > > > > > Run-All>
 > > > > > > > > > > > >> contains this test failed.
Is it new or not new?>
 > > > Actually>
 > > > > we>
 > > > > > > > don't>
 > > > > > > > > > > know.>
 > > > > > > > > > > > >>>
 > > > > > > > > > > > >> Existing requirement is about
all TC must be 
green, so>
 > > > > let's>
 > > > > > > > keep>
 > > > > > > > > it>
 > > > > > > > > > > as>
 > > > > > > > > > > > is.>
 > > > > > > > > > > > >>>
 > > > > > > > > > > > >> ср, 23 мая 2018 г.
в 17:02, Vladimir Ozerov <>
 > > > > > > > vozerov@gridgain.com>
 > > > > > > > > >:>
 > > > > > > > > > > > >>>
 > > > > > > > > > > > >>> Igniters,>
 > > > > > > > > > > > >>>>
 > > > > > > > > > > > >>> I created review checklist
on WIKI [1] and 
also fixed>
 > > > > > related>
 > > > > > > > > pages>
 > > > > > > > > > > > (e.g.>
 > > > > > > > > > > > >>> "How To Contribute").
Please let me know if 
you have>
 > > > any>
 > > > > > > > comments>
 > > > > > > > > > > > before>
 > > > > > > > > > > > >> I>
 > > > > > > > > > > > >>> go with public announce.>
 > > > > > > > > > > > >>>>
 > > > > > > > > > > > >>> Vladimir.>
 > > > > > > > > > > > >>>>
 > > > > > > > > > > > >>> [1]>
 > > > > > > > > > > >>
 > > > > > > >>
 > > > https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist>
 > > > > > > > > > > > >>>>
 > > > > > > > > > > > >>> On Thu, May 10, 2018
at 5:10 PM, Vladimir 
Ozerov <>
 > > > > > > > > > > vozerov@gridgain.com>
 > > > > > > > > > > > >>
 > > > > > > > > > > > >>> wrote:>
 > > > > > > > > > > > >>>>
 > > > > > > > > > > > >>>> Ilya,>
 > > > > > > > > > > > >>>>>
 > > > > > > > > > > > >>>> We define that exception
messages *SHOULD* 
have>
 > > clear>
 > > > > > > > > explanation>
 > > > > > > > > > on>
 > > > > > > > > > > > >> what>
 > > > > > > > > > > > >>>> is wrong. *SHOULD*
mean that the rule should 
be>
 > > > followed>
 > > > > > > > unless>
 > > > > > > > > > > there>
 > > > > > > > > > > > >> is>
 > > > > > > > > > > > >>> a>
 > > > > > > > > > > > >>>> reason not to follow.
In your case you refer 
to some>
 > > > > > > > unexpected>
 > > > > > > > > > > > >> behavior.>
 > > > > > > > > > > > >>>> I.e. an exceptional
situation developer is 
not aware>
 > > > of.>
 > > > > > In>
 > > > > > > > this>
 > > > > > > > > > > case>
 > > > > > > > > > > > >> for>
 > > > > > > > > > > > >>>> sure we cannot force
contributor to explain 
what is>
 > > > > wrong,>
 > > > > > > > > > because,>
 > > > > > > > > > > > >> well,>
 > > > > > > > > > > > >>>> we don't know. This
is why we relaxed the 
rule from>
 > > > > *MUST*>
 > > > > > > to>
 > > > > > > > > > > > *SHOULD*.>
 > > > > > > > > > > > >>>>>
 > > > > > > > > > > > >>>> On Thu, May 10, 2018
at 3:50 PM, Ilya 
Kasnacheev <>
 > > > > > > > > > > > >>>> ilya.kasnacheev@gmail.com>
wrote:>
 > > > > > > > > > > > >>>>>
 > > > > > > > > > > > >>>>> I don't think
I quite understand how 
exception>
 > > > > > explanations>
 > > > > > > > > > should>
 > > > > > > > > > > > >> work.>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> Imagine we have
the following exception:>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> // At least RuntimeException
can be thrown 
by the>
 > > > code>
 > > > > > > above>
 > > > > > > > > when>
 > > > > > > > > > > > >>>>> GridCacheContext
is cleaned and there is>
 > > > > > > > > > > > >>>>> // an attempt
to use cleaned resources.>
 > > > > > > > > > > > >>>>> U.error(log,
"Unexpected exception during 
cache>
 > > > > update",>
 > > > > > > e);>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> I mean, we genuinely
don't know what 
happened here.>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> Under new rules,
what kind of "workaround" 
would>
 > > that>
 > > > > > > > exception>
 > > > > > > > > > > > >> suggest?>
 > > > > > > > > > > > >>>>> "Try turning
it off and then back on"?>
 > > > > > > > > > > > >>>>> What explanation
how to resolve this 
exception can>
 > > we>
 > > > > > > offer?>
 > > > > > > > > > > "Please>
 > > > > > > > > > > > >>> write>
 > > > > > > > > > > > >>>>> to dev@apache.ignite.org
or to Apache JIRA, 
and>
 > > then>
 > > > > > wait>
 > > > > > > > for>
 > > > > > > > > a>
 > > > > > > > > > > > >> release>
 > > > > > > > > > > > >>>>> with fix?">
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> I'm really confused
how we can implement 
1.6 and>
 > > 1.7>
 > > > > when>
 > > > > > > > > dealing>
 > > > > > > > > > > > with>
 > > > > > > > > > > > >>>>> messy real-world
code.>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> Regards,>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> -->
 > > > > > > > > > > > >>>>> Ilya Kasnacheev>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>> 2018-05-10 11:39
GMT+03:00 Vladimir Ozerov <>
 > > > > > > > > vozerov@gridgain.com>
 > > > > > > > > > >:>
 > > > > > > > > > > > >>>>>>
 > > > > > > > > > > > >>>>>> Andrey, Anton,
Alex>
 > > > > > > > > > > > >>>>>>>
 > > > > > > > > > > > >>>>>> Agree, *SHOULD*
is more appropriate here.>
 > > > > > > > > > > > >>>>>>>
 > > > > > > > > > > > >>>>>> Please see
latest version below. Does 
anyone want>
 > > to>
 > > > > add>
 > > > > > > or>
 > > > > > > > > > change>
 > > > > > > > > > > > >>>>>> something?
Let's wait for several days for 
more>
 > > > > feedback>
 > > > > > > and>
 > > > > > > > > > then>
 > > > > > > > > > > > >>>>> publish>
 > > > > > > > > > > > >>>>>> and announce
t

Mime
View raw message