ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vladimir Ozerov <voze...@gridgain.com>
Subject Re: Ticket review checklist
Date Thu, 16 Aug 2018 08:21:57 GMT
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 <vololo100@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
> <dpavlov.spb@gmail.com>
>  > > >>
>  > > > > > wrote:>
>  > > > > >>
>  > > > > > > Vladimir, I mean strict definition, how much previous
runs
> should>
>  > > > > > > contributor consider? What if test was failed by
> infrastructure>
>  > > > reason>
>  > > > > in>
>  > > > > > > master previously, how can contributor be sure test failure
> !=>
>  > > broken>
>  > > > > > code>
>  > > > > > > in PR? In this case it should be double checked by>
>  > > > > contributor/reviewer.>
>  > > > > > > I'm sure nobody can give strict definition of 'new' failure.>
>  > > > > > >>
>  > > > > > > Flaky tests detected by TC may be taken into account in
> check-list,>
>  > > > > > because>
>  > > > > > > contributor can check if failure is flaky. But again,
not
> all tests>
>  > > > > with>
>  > > > > > > floating failure is detected by TC as flaky.>
>  > > > > > >>
>  > > > > > > I don't understand what problem will be solved if we soften
> current>
>  > > > > > > requirement with 'new' test? Everybody will continue to
> complain>
>  > > they>
>  > > > > > PR's>
>  > > > > > > test failures is not `new`. So let's keep it as is.>
>  > > > > > >>
>  > > > > > > пн, 4 июн. 2018 г. в 14:46, Vladimir Ozerov
> <vozerov@gridgain.com>
>  > > >:>
>  > > > > > >>
>  > > > > > > > Dmitry,>
>  > > > > > > >>
>  > > > > > > > New failure is a failure hasn't happened on previous
> runs. If it>
>  > > do>
>  > > > > > > > happened, then contributor should see if it is a
flaky or
> not>
>  > > > through>
>  > > > > > > local>
>  > > > > > > > and TC runs. The same works for timeout suites.>
>  > > > > > > > Current statement in "Review Checklist" that there
are
> should be>
>  > > no>
>  > > > > > > failed>
>  > > > > > > > tests is not applicable to real word. Almost every
patch is>
>  > > pushed>
>  > > > to>
>  > > > > > > > repository with test failures.>
>  > > > > > > >>
>  > > > > > > > On Mon, Jun 4, 2018 at 2:22 PM, Dmitry Pavlov <>
>  > > > dpavlov.spb@gmail.com>
>  > > > > >>
>  > > > > > > > wrote:>
>  > > > > > > >>
>  > > > > > > > > Hi Vladimir, could you provide definition what
is new
> failure?>
>  > > > how>
>  > > > > do>
>  > > > > > > you>
>  > > > > > > > > know it is new or not?>
>  > > > > > > > >>
>  > > > > > > > > And please forget for a moment you're Ignite
expert &
> veteran,>
>  > > > > > imagine>
>  > > > > > > > you>
>  > > > > > > > > are newcomer.>
>  > > > > > > > >>
>  > > > > > > > > I can't find any criteria that can be used by
newbie to
> come to>
>  > > > the>
>  > > > > > > > > conclusion that test is new. Patch is accepted
by
> reviewer, so>
>  > > it>
>  > > > > > > should>
>  > > > > > > > be>
>  > > > > > > > > up to him to correctly register failures in
tickets with>
>  > > > > > > > > MakeTeamCityGreenAgain label and mute unimportant
tests.>
>  > > > > > > > >>
>  > > > > > > > > пн, 4 июн. 2018 г. в 11:32, Vladimir
Ozerov <>
>  > > > vozerov@gridgain.com>
>  > > > > >:>
>  > > > > > > > >>
>  > > > > > > > > > Dmitry,>
>  > > > > > > > > >>
>  > > > > > > > > > I still do not see how new patches could
be accepted
> with>
>  > > this>
>  > > > > > > > > requirement>
>  > > > > > > > > > in place. Consider the following case:
I created a
> patch and>
>  > > > run>
>  > > > > it>
>  > > > > > > on>
>  > > > > > > > > TC,>
>  > > > > > > > > > observed N failures, verified through TC
history that
> none if>
>  > > > > them>
>  > > > > > > are>
>  > > > > > > > > new.>
>  > > > > > > > > > Am I eligible to push the commit?>
>  > > > > > > > > >>
>  > > > > > > > > > On Thu, May 24, 2018 at 3:11 PM, Dmitry
Pavlov <>
>  > > > > > > dpavlov.spb@gmail.com>>
>  > > > > > > > > > wrote:>
>  > > > > > > > > >>
>  > > > > > > > > > > Petr, good point. It is more intuitive,
we should
> mark test>
>  > > > we>
>  > > > > > can>
>  > > > > > > > > ignore>
>  > > > > > > > > > > by mute.>
>  > > > > > > > > > >>
>  > > > > > > > > > > So Vladimir, you or other Ignite veteran
can mute
> test, if>
>  > > > can>
>  > > > > > say>
>  > > > > > > it>
>  > > > > > > > > is>
>  > > > > > > > > > > not important.>
>  > > > > > > > > > >>
>  > > > > > > > > > > чт, 24 мая 2018 г. в 15:07,
Petr Ivanov <>
>  > > mr.weider@gmail.com>
>  > > > >:>
>  > > > > > > > > > >>
>  > > > > > > > > > > > Why cannot we mute (and file
corresponding
> tickets) all>
>  > > > test>
>  > > > > > > > failures>
>  > > > > > > > > > > > (including flaky) to some date
and start
> initiative Green>
>  > > > TC?>
>  > > > > > > > > > > >>
>  > > > > > > > > > > >>
>  > > > > > > > > > > >>
>  > > > > > > > > > > > > On 24 May 2018, at 15:04,
Vladimir Ozerov <>
>  > > > > > > vozerov@gridgain.com>>
>  > > > > > > > > > > wrote:>
>  > > > > > > > > > > > >>
>  > > > > > > > > > > > > Dmitry,>
>  > > > > > > > > > > > >>
>  > > > > > > > > > > > > We cannot add this requirements,
because we do
> have>
>  > > > > failures>
>  > > > > > on>
>  > > > > > > > TC.>
>  > > > > > > > > > > This>
>  > > > > > > > > > > > > requirement implies that
all development would
> stop>
>  > > until>
>  > > > > TC>
>  > > > > > is>
>  > > > > > > > > > green.>
>  > > > > > > > > > > > > We never had old requirement
work, neither we
> need to>
>  > > > > enforce>
>  > > > > > > it>
>  > > > > > > > > now.>
>  > > > > > > > > > > > >>
>  > > > > > > > > > > > > On Thu, May 24, 2018 at
2:59 PM, Dmitry Pavlov <>
>  > > > > > > > > > dpavlov.spb@gmail.com>>
>  > > > > > > > > > > > > wrote:>
>  > > > > > > > > > > > >>
>  > > > > > > > > > > > >> 3.c>
>  > > > > > > > > > > > >>>
>  > > > > > > > > > > > >> 1. All test suites *MUST*
be run on TeamCity [3]>
>  > > > before>
>  > > > > > > merge>
>  > > > > > > > to>
>  > > > > > > > > > > > master,>
>  > > > > > > > > > > > >> there *MUST NOT* be
any test failures>
>  > > > > > > > > > > > >>>
>  > > > > > > > > > > > >>>
>  > > > > > > > > > > > >> 'New' word should be
removed because we cant
> separate>
>  > > > > `new`>
>  > > > > > > and>
>  > > > > > > > > `non>
>  > > > > > > > > > > > new`>
>  > > > > > > > > > > > >> failures.>
>  > > > > > > > > > > > >>>
>  > > > > > > > > > > > >> Let's imagine example,
we have 50 green runs in>
>  > > master.>
>  > > > > And>
>  > > > > > PR>
>  > > > > > > > > > Run-All>
>  > > > > > > > > > > > >> contains this test failed.
Is it new or not new?>
>  > > > Actually>
>  > > > > we>
>  > > > > > > > don't>
>  > > > > > > > > > > know.>
>  > > > > > > > > > > > >>>
>  > > > > > > > > > > > >> Existing requirement
is about all TC must be
> green, so>
>  > > > > let's>
>  > > > > > > > keep>
>  > > > > > > > > it>
>  > > > > > > > > > > as>
>  > > > > > > > > > > > is.>
>  > > > > > > > > > > > >>>
>  > > > > > > > > > > > >> ср, 23 мая 2018
г. в 17:02, Vladimir Ozerov <>
>  > > > > > > > vozerov@gridgain.com>
>  > > > > > > > > >:>
>  > > > > > > > > > > > >>>
>  > > > > > > > > > > > >>> Igniters,>
>  > > > > > > > > > > > >>>>
>  > > > > > > > > > > > >>> I created review
checklist on WIKI [1] and
> also fixed>
>  > > > > > related>
>  > > > > > > > > pages>
>  > > > > > > > > > > > (e.g.>
>  > > > > > > > > > > > >>> "How To Contribute").
Please let me know if
> you have>
>  > > > any>
>  > > > > > > > comments>
>  > > > > > > > > > > > before>
>  > > > > > > > > > > > >> I>
>  > > > > > > > > > > > >>> go with public announce.>
>  > > > > > > > > > > > >>>>
>  > > > > > > > > > > > >>> Vladimir.>
>  > > > > > > > > > > > >>>>
>  > > > > > > > > > > > >>> [1]>
>  > > > > > > > > > > >>
>  > > > > > > >>
>  > > > https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist
> >
>  > > > > > > > > > > > >>>>
>  > > > > > > > > > > > >>> On Thu, May 10,
2018 at 5:10 PM, Vladimir
> Ozerov <>
>  > > > > > > > > > > vozerov@gridgain.com>
>  > > > > > > > > > > > >>
>  > > > > > > > > > > > >>> wrote:>
>  > > > > > > > > > > > >>>>
>  > > > > > > > > > > > >>>> Ilya,>
>  > > > > > > > > > > > >>>>>
>  > > > > > > > > > > > >>>> We define that
exception messages *SHOULD*
> have>
>  > > clear>
>  > > > > > > > > explanation>
>  > > > > > > > > > on>
>  > > > > > > > > > > > >> what>
>  > > > > > > > > > > > >>>> is wrong. *SHOULD*
mean that the rule should
> be>
>  > > > followed>
>  > > > > > > > unless>
>  > > > > > > > > > > there>
>  > > > > > > > > > > > >> is>
>  > > > > > > > > > > > >>> a>
>  > > > > > > > > > > > >>>> reason not to
follow. In your case you refer
> to some>
>  > > > > > > > unexpected>
>  > > > > > > > > > > > >> behavior.>
>  > > > > > > > > > > > >>>> I.e. an exceptional
situation developer is
> not aware>
>  > > > of.>
>  > > > > > In>
>  > > > > > > > this>
>  > > > > > > > > > > case>
>  > > > > > > > > > > > >> for>
>  > > > > > > > > > > > >>>> sure we cannot
force contributor to explain
> what is>
>  > > > > wrong,>
>  > > > > > > > > > because,>
>  > > > > > > > > > > > >> well,>
>  > > > > > > > > > > > >>>> we don't know.
This is why we relaxed the
> rule from>
>  > > > > *MUST*>
>  > > > > > > to>
>  > > > > > > > > > > > *SHOULD*.>
>  > > > > > > > > > > > >>>>>
>  > > > > > > > > > > > >>>> On Thu, May
10, 2018 at 3:50 PM, Ilya
> Kasnacheev <>
>  > > > > > > > > > > > >>>> ilya.kasnacheev@gmail.com>
wrote:>
>  > > > > > > > > > > > >>>>>
>  > > > > > > > > > > > >>>>> I don't
think I quite understand how
> exception>
>  > > > > > explanations>
>  > > > > > > > > > should>
>  > > > > > > > > > > > >> work.>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> Imagine
we have the following exception:>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> // At least
RuntimeException can be thrown
> by the>
>  > > > code>
>  > > > > > > above>
>  > > > > > > > > when>
>  > > > > > > > > > > > >>>>> GridCacheContext
is cleaned and there is>
>  > > > > > > > > > > > >>>>> // an attempt
to use cleaned resources.>
>  > > > > > > > > > > > >>>>> U.error(log,
"Unexpected exception during
> cache>
>  > > > > update",>
>  > > > > > > e);>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> I mean,
we genuinely don't know what
> happened here.>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> Under new
rules, what kind of "workaround"
> would>
>  > > that>
>  > > > > > > > exception>
>  > > > > > > > > > > > >> suggest?>
>  > > > > > > > > > > > >>>>> "Try turning
it off and then back on"?>
>  > > > > > > > > > > > >>>>> What explanation
how to resolve this
> exception can>
>  > > we>
>  > > > > > > offer?>
>  > > > > > > > > > > "Please>
>  > > > > > > > > > > > >>> write>
>  > > > > > > > > > > > >>>>> to dev@apache.ignite.org
or to Apache JIRA,
> and>
>  > > then>
>  > > > > > wait>
>  > > > > > > > for>
>  > > > > > > > > a>
>  > > > > > > > > > > > >> release>
>  > > > > > > > > > > > >>>>> with fix?">
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> I'm really
confused how we can implement
> 1.6 and>
>  > > 1.7>
>  > > > > when>
>  > > > > > > > > dealing>
>  > > > > > > > > > > > with>
>  > > > > > > > > > > > >>>>> messy real-world
code.>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> Regards,>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> -->
>  > > > > > > > > > > > >>>>> Ilya Kasnacheev>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>> 2018-05-10
11:39 GMT+03:00 Vladimir Ozerov <>
>  > > > > > > > > vozerov@gridgain.com>
>  > > > > > > > > > >:>
>  > > > > > > > > > > > >>>>>>
>  > > > > > > > > > > > >>>>>> Andrey,
Anton, Alex>
>  > > > > > > > > > > > >>>>>>>
>  > > > > > > > > > > > >>>>>> Agree,
*SHOULD* is more appropriate here.>
>  > > > > > > > > > > > >>>>>>>
>  > > > > > > > > > > > >>>>>> Please
see latest version below. Does
> anyone want>
>  > > to>
>  > > > > add>
>  > > > > > > or>
>  > > > > > > > > > change>
>  > > > > > > > > > > > >>>>>> something?
Let's wait for several days for
> more>
>  > > > > feedback>
>  > > > > > > and>
>  > > > > > > > > > then>
>  > > > > > > > > > > > >>>>> publish>
>  > > > > > > > > > > > >>>>>> and
announce t
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message