You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by ipavlukhin <vo...@gmail.com> on 2018/08/14 16:16:41 UTC
Re: Ticket review checklist
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
<dp...@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
<vo...@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
Re: Ticket review checklist
Posted by Павлухин Иван <vo...@gmail.com>.
Hi Dmitriy,
I agree with you about lambdas. For me they are quite useful and I believe
that this language feature is a solid and well proven part of modern Java.
I still feel that current statement in our guidelines should be rephrased.
But if others are ok with it then let's keep it as is.
2018-08-16 16:47 GMT+03:00 Dmitriy Pavlov <dp...@gmail.com>:
> Hi Ivan,
>
> Unfortunately, the review checklist does not work as well as it could. I
> hope the situation will change in the nearest future, I think we should
> come back to this idea and encourage contributors and reviewers to use the
> list.
>
> As for lambda's: some Igniters feel confident about it, and some Igniters
> don't. My opinion it is perfectly ok to use it if usage is local node only,
> is there is no chance lambda is serialized to the network. If there is such
> chance it is better to avoid it.
>
> Sincerely,
> Dmitriy Pavlov
>
> чт, 16 авг. 2018 г. в 12:09, Павлухин Иван <vo...@gmail.com>:
>
> > Vladimir,
> >
> > First of all, statements in Java 8 section [1] looks kind of prohibitive
> > for me. When a new contributor see words "preferred" and "avoided in most
> > cases" he most likely will not use such features (like I did). If a
> > statement is not prohibitive in practice it could be at least rephrased.
> >
> > A bit about expressiveness. I written a code during working on a real
> > ticket. The case is quite common in Ignite codebase. You can find example
> > with couple of approaches in snippet [2]. For me approach with lambdas is
> > expressive, compact and simple.
> >
> > What do you think?
> >
> > [1]
> >
> > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#
> CodingGuidelines-Java8
> > [2] https://gist.github.com/pavlukhin/92701277f66f8901a7feda6283a5a299
> >
> > 2018-08-16 11:21 GMT+03:00 Vladimir Ozerov <vo...@gridgain.com>:
> >
> > > Hi Ivan,
> > >
> > > From what I see we do not restrict contributors to use lambdas and
> > streams.
> > > Document states that plain collections and anonymous classes are
> > > *preferred*. This is not obligatory requirement, and it seems
> reasonable
> > to
> > > me, because when developing complex projects at times it is better to
> > have
> > > more expressive code, than less non-obvious code which makes dozens
> > > operations in a single string.
> > >
> > > Or may be there are any other statements in the checklist which
> prevents
> > > users from using Java 8 features?
> > >
> > > Vladimir.
> > >
> > > On Tue, Aug 14, 2018 at 7:16 PM ipavlukhin <vo...@gmail.com>
> wrote:
> > >
> > > > 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
> > > > <dp...@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
> > > > <vo...@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
> > > >
> > >
> >
> >
> >
> > --
> > Best regards,
> > Ivan Pavlukhin
> >
>
--
Best regards,
Ivan Pavlukhin
Re: Ticket review checklist
Posted by Dmitriy Pavlov <dp...@gmail.com>.
Hi Ivan,
Unfortunately, the review checklist does not work as well as it could. I
hope the situation will change in the nearest future, I think we should
come back to this idea and encourage contributors and reviewers to use the
list.
As for lambda's: some Igniters feel confident about it, and some Igniters
don't. My opinion it is perfectly ok to use it if usage is local node only,
is there is no chance lambda is serialized to the network. If there is such
chance it is better to avoid it.
Sincerely,
Dmitriy Pavlov
чт, 16 авг. 2018 г. в 12:09, Павлухин Иван <vo...@gmail.com>:
> Vladimir,
>
> First of all, statements in Java 8 section [1] looks kind of prohibitive
> for me. When a new contributor see words "preferred" and "avoided in most
> cases" he most likely will not use such features (like I did). If a
> statement is not prohibitive in practice it could be at least rephrased.
>
> A bit about expressiveness. I written a code during working on a real
> ticket. The case is quite common in Ignite codebase. You can find example
> with couple of approaches in snippet [2]. For me approach with lambdas is
> expressive, compact and simple.
>
> What do you think?
>
> [1]
>
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Java8
> [2] https://gist.github.com/pavlukhin/92701277f66f8901a7feda6283a5a299
>
> 2018-08-16 11:21 GMT+03:00 Vladimir Ozerov <vo...@gridgain.com>:
>
> > Hi Ivan,
> >
> > From what I see we do not restrict contributors to use lambdas and
> streams.
> > Document states that plain collections and anonymous classes are
> > *preferred*. This is not obligatory requirement, and it seems reasonable
> to
> > me, because when developing complex projects at times it is better to
> have
> > more expressive code, than less non-obvious code which makes dozens
> > operations in a single string.
> >
> > Or may be there are any other statements in the checklist which prevents
> > users from using Java 8 features?
> >
> > Vladimir.
> >
> > On Tue, Aug 14, 2018 at 7:16 PM ipavlukhin <vo...@gmail.com> wrote:
> >
> > > 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
> > > <dp...@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
> > > <vo...@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
> > >
> >
>
>
>
> --
> Best regards,
> Ivan Pavlukhin
>
Re: Ticket review checklist
Posted by Павлухин Иван <vo...@gmail.com>.
Vladimir,
First of all, statements in Java 8 section [1] looks kind of prohibitive
for me. When a new contributor see words "preferred" and "avoided in most
cases" he most likely will not use such features (like I did). If a
statement is not prohibitive in practice it could be at least rephrased.
A bit about expressiveness. I written a code during working on a real
ticket. The case is quite common in Ignite codebase. You can find example
with couple of approaches in snippet [2]. For me approach with lambdas is
expressive, compact and simple.
What do you think?
[1]
https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines#CodingGuidelines-Java8
[2] https://gist.github.com/pavlukhin/92701277f66f8901a7feda6283a5a299
2018-08-16 11:21 GMT+03:00 Vladimir Ozerov <vo...@gridgain.com>:
> Hi Ivan,
>
> From what I see we do not restrict contributors to use lambdas and streams.
> Document states that plain collections and anonymous classes are
> *preferred*. This is not obligatory requirement, and it seems reasonable to
> me, because when developing complex projects at times it is better to have
> more expressive code, than less non-obvious code which makes dozens
> operations in a single string.
>
> Or may be there are any other statements in the checklist which prevents
> users from using Java 8 features?
>
> Vladimir.
>
> On Tue, Aug 14, 2018 at 7:16 PM ipavlukhin <vo...@gmail.com> wrote:
>
> > 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
> > <dp...@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
> > <vo...@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
> >
>
--
Best regards,
Ivan Pavlukhin
Re: Ticket review checklist
Posted by Vladimir Ozerov <vo...@gridgain.com>.
Hi Ivan,
From what I see we do not restrict contributors to use lambdas and streams.
Document states that plain collections and anonymous classes are
*preferred*. This is not obligatory requirement, and it seems reasonable to
me, because when developing complex projects at times it is better to have
more expressive code, than less non-obvious code which makes dozens
operations in a single string.
Or may be there are any other statements in the checklist which prevents
users from using Java 8 features?
Vladimir.
On Tue, Aug 14, 2018 at 7:16 PM ipavlukhin <vo...@gmail.com> wrote:
> 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
> <dp...@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
> <vo...@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
>