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....@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
<voze...@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....@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 <>
> > > voze...@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....@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.wei...@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 <>
> > > > > > voze...@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....@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 <>
> > > > > > > voze...@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 <>
> > > > > > > > > > voze...@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.kasnach...@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 d...@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 <>
> > > > > > > > voze...@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
