Re: Ticket review checklist

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.

Re: Ticket review checklist

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

Re: Ticket review checklist

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.

Re: Ticket review checklist

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

Re: Ticket review checklist

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?

Re: Ticket review checklist

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)

Re: Ticket review checklist

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 г. в

Re: Ticket review checklist

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

Re: Ticket review checklist

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

Re: Ticket review checklist

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 wrote: > Vladimir, I mean strict definition, how much previous runs should > contributor consider? What if test was failed by infrastructure reason in > master

Re: Ticket review checklist

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

Re: Ticket review checklist

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

Re: Ticket review checklist

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

Re: Ticket review checklist

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

Re: Ticket review checklist

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 : > Why cannot we mute (and file corresponding tickets) all

Re: Ticket review checklist

We cannot change this requirement to be softer because we need to come to sutuation of 0-failed test. If we allow commit with test failures, there will be a lot of mistakes new failures will be considered as existing. All contributors will check only new/not new failures. But actually all

Re: Ticket review checklist

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 wrote: > > Dmitry, > > We cannot add this requirements, because we do have failures on TC.

Re: Ticket review checklist

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

Re: Ticket review checklist

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

Re: Ticket review checklist

Dmitry, Could you please formulate this requirement? On Wed, May 23, 2018 at 5:06 PM, Dmitry Pavlov wrote: > Hi Vladimir, > > I've replied in separate thread. > > I would like to keep requirement of Green TC instead of separation to > new/old test failures. > > Once you

Re: Ticket review checklist

Hi Vladimir, I've replied in separate thread. I would like to keep requirement of Green TC instead of separation to new/old test failures. Once you allow just one test failure, it would be more failures after some time. Sincererly, Dmitriy Pavlov ср, 23 мая 2018 г. в 17:02, Vladimir Ozerov

Re: Ticket review checklist

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

Re: Ticket review checklist

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

Re: Ticket review checklist

Correct message should be added to original exception thrown at cache update. Re-thrown exception case is not counter-example for general approach. чт, 10 мая 2018 г. в 16:24, Anton Vinogradov : > >> U.error(log, "Unexpected exception during cache update", e); > >> I mean, we

Re: Ticket review checklist

>> U.error(log, "Unexpected exception during cache update", e); >> I mean, we genuinely don't know what happened here. Fully agree with Ilya. In some cases it's impossible to explain the reason. All we can is just to log the error. >> Error text `Unexpected exception during cache update` is

Re: Ticket review checklist

Dmitry, can you please perform an excercise of looking that piece of code up (org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridDhtAtomicCache), telling us how an ideal message should look like in this case? Thanks, -- Ilya Kasnacheev 2018-05-10 16:15 GMT+03:00 Dmitry

Re: Ticket review checklist

Hi Ilya, Error text `Unexpected exception during cache update` is brilliant example how product should not behave. So requiring contributors to explain reasons of failure would be good first step to come to situation that exception text is so clear that user will know what to do. It will

Re: Ticket review checklist

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

Re: Ticket review checklist

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 this list. Note that it would not be carved in stone and we will be able to

Re: Ticket review checklist

Dima, It will be on WIKI once community is in agreement on it's content. On Mon, May 7, 2018 at 11:09 PM, Dmitriy Setrakyan wrote: > Is this list on the Wiki? > > On Mon, May 7, 2018 at 7:26 AM, Vladimir Ozerov > wrote: > > > Igniters, > > > > This

Re: Ticket review checklist

Anton, Checklist is not about checking, but about expectations and trust. Implementor should know what is expected from his patch. And reviewer is not inspector. His goal is to agree with implementer that checklist requirements s addressed. Consider thin client protocol compatibility. Without

Re: Ticket review checklist

Anton, I agree, *MUST* for exception reasons and *SHOULD* for ways of resolution sound clearer. 2018-05-08 12:56 GMT+03:00 Anton Vinogradov : > Andrey, > > How about > 1.6) All exceptions thrown to a user *MUST* have explanation of workaround > and contain original error. > All

Re: Ticket review checklist

Andrey, How about 1.6) All exceptions thrown to a user *MUST* have explanation of workaround and contain original error. All exceptions thrown to a user *SHOULD* have explanation how to resolve if possible. ? вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov : > Vladimir, checklist

Re: Ticket review checklist

Vladimir, checklist looks pleasant enough for me. I'd like to suggest one minor change. In 1.6 *MUST* seems to be too strict, *SHOULD* would be enough. It can be frustrating for API user if I explain how to fix NPEs in a trivial way, for example. 2018-05-08 11:34 GMT+03:00 Anton Vinogradov

Re: Ticket review checklist

Alex, It is not sounds like that, obviously. Tests should cover all negative and positive cases. You should add enough tests to cover all cases. Sometimes one test can cover more than one case, so two tests *CAN* partially check same things. In case some cases already covered you should not

Re: Ticket review checklist

Vladimir, the 3.1 is a bit unclear for me. Which code coverage is acceptable? Now it sounds like two tests are enough (one for positive and one for negative cases). 2018-05-07 23:09 GMT+03:00 Dmitriy Setrakyan : > Is this list on the Wiki? > > On Mon, May 7, 2018 at 7:26

Re: Ticket review checklist

Is this list on the Wiki? On Mon, May 7, 2018 at 7:26 AM, Vladimir Ozerov wrote: > Igniters, > > This is the checklist I have at the moment. Please let me know if you have > any comments on existing items, or want to add or remove anything. It looks > like we may have not

Re: Ticket review checklist

Vova, Looks good to me. Please add clear explanation how to check 1.4, 1.5 and 2.x. Also, this should be published as a wiki page with refs to... eg. Coding Guidelines. пн, 7 мая 2018 г. в 17:26, Vladimir Ozerov : > Igniters, > > This is the checklist I have at the

Re: Ticket review checklist

Igniters, This is the checklist I have at the moment. Please let me know if you have any comments on existing items, or want to add or remove anything. It looks like we may have not only strict rules, but *nice to have* points here as well with help of *MUST*, *SHOULD* and *MAY* words as per

Re: Ticket review checklist

Hi Dmitry, Yes, I'll do that in the nearest days. On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov wrote: > Igniters, the idea was related to small refactorings co-located with main > change. > > Main change itself indicates that existing code did not meet the criteria >

Re: Ticket review checklist

Igniters, the idea was related to small refactorings co-located with main change. Main change itself indicates that existing code did not meet the criteria of practice. Approving of standalone refactorings instead contradicts with principle don't touch if it works. So I still like idea of

Re: Ticket review checklist

+1 to Vladimir Ozerov 2018-04-25 11:44 GMT+03:00 Vladimir Ozerov : > Guys, > > The problem with in-place refactorings is that you increase affected scope. > It is not uncommon to break compatibility or public contracts with even > minor things. E.g. recently we decided drop

Re: Ticket review checklist

Guys, The problem with in-place refactorings is that you increase affected scope. It is not uncommon to break compatibility or public contracts with even minor things. E.g. recently we decided drop org.jsr166 package in favor of Java 8 classes. Innocent change. Result - broken storage. Another

Re: Ticket review checklist

+1. Once again, I beg for "small refactoring permission" in a checklist. As of today, separate tickets for small refactorings has lowest priority, since they neither fix any flaw nor add new functionality. Also, the attempts to make issue-related code safer / cleaner / more readable in "real"

Re: Ticket review checklist

Vladimir, I am not talking about massive/sophisticated refactoring. But I believe that ask to extract some methods should be OK to do without an extra ticket. A checklist shouldn't be necessarily a set of certain rules but also it could include suggestion and reminders. On Tue, Apr 24, 2018 at

Re: Ticket review checklist

Ed, Refactoring is a separate task. If you would like to rework exchange future - please do this in a ticket "Refactor exchange task", nobody would against this. This is just a matter of creating separate ticket and separate PR. If one have a time for refactoring, it should not be a problem for

Re: Ticket review checklist

Igniters, I don't understand why you are so against refactoring. Code already smells like hell. Methods 200+ line is normal. Exchange future is asking to be separated on several one. Transaction code could understand few people. If we separate refactoring from development it would mean that no

Re: Ticket review checklist

Hi Igniters, +1 to idea of checklist. +1 to refactoring and documenting code related to ticket in +/-20 LOC at least. If we start to do it as part of our regular contribution, code will be better, it would became common practice and part of Apache Ignite development culure. If we will hope we

Re: Ticket review checklist

4) Metrics. partially +1 It makes sense to have some minimal code coverage for new code in PR. IMHO. Also, we can limit the cyclomatic complexity of the new code in PR too. 6) Refactoring -1 I understand why people want to refactor old code. But I think refactoring should be always a separate

Re: Ticket review checklist

What about adding the following item to the checklist: when the change adds new functionality, then unit tests should also be provided, if it's technically possible? As for refactorings, in fact they are strongly discouraged today for some unclear reason. Let's permit to make refactorings in the

Re: Ticket review checklist

Hi Ed, Unfortunately some of these points are not good candidates for the checklist because of these: - It must be clear and disallow *multiple interpretations* - It must be *lightweight*, otherwise Ignite development would become a nightmare We cannot have "nice to have" points here. Checklist

Re: Ticket review checklist

Also, I want to add some technical requirement. Let's discuss them. 1) Code style. The code needs to be formatted according to coding guidelines . The code must not contain TODOs without a ticket reference. It is highly

Re: Ticket review checklist

Hi, guys. I believe that we should update maintainers list before adding this review requirement. There should not be the situation when there is only one contributor who is responsible for a component. We already have issues with review speed and response time. On Fri, Apr 20, 2018 at 2:17 PM,

Re: Ticket review checklist

Vova, Everything you described sound good to me. I'd like to propose to create special page at AI Wiki and to describe checklist. In case we'll find something should be changed/improved it will be easy to update the page. 2018-04-20 0:53 GMT+03:00 Nikolay Izhikov : >

Re: Ticket review checklist

Hello, Vladimir. Thank you for seting up this discussion. As we discussed, I think an important part of this check list is compatibility rules. * What should be backward compatible? * How should we maintain it? > 3) If ticket changes public API or existing behavior, at least two commiters >