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