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 <dpavlov....@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, Павлухин Иван <vololo...@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 <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 > > > -- Best regards, Ivan Pavlukhin
