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 <voze...@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 <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 > > > -- Best regards, Ivan Pavlukhin
