ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Павлухин Иван <vololo...@gmail.com>
Subject Re: Ticket review checklist
Date Thu, 16 Aug 2018 09:09:18 GMT
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 <vozerov@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 <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
> >
>



-- 
Best regards,
Ivan Pavlukhin

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