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.

2018-08-16 16:47 GMT+03:00 Dmitriy Pavlov :

> 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, Павлухин Иван :
>
> > 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 :
> >
> > > 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 
> 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  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  ···>
> > > >  >
> > > >  > 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=buildTypeHistoryList_
> > > IgniteTests24Java8=%3Cdefault%3E>
> > > >
> > > >
> > > >  >
> > > >  >
> > > >  > пн, 9 июл. 2018 г. в 22:29, Anton Vinogradov :>
> > > >  >
> > > >  > > 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 :>
> > > >  > >>
> > > >  > > > Hi 

Re: Ticket review checklist

[Dmitriy Pavlov]

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, Павлухин Иван :

> 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 :
>
> > 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  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  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  ···>
> > >  >
> > >  > 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=buildTypeHistoryList_
> > IgniteTests24Java8=%3Cdefault%3E>
> > >
> > >
> > >  >
> > >  >
> > >  > пн, 9 июл. 2018 г. в 22:29, Anton Vinogradov :>
> > >  >
> > >  > > 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 :>
> > >  > >>
> > >  > > > 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 

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.

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 :

> 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  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  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  ···>
> >  >
> >  > 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=buildTypeHistoryList_
> IgniteTests24Java8=%3Cdefault%3E>
> >
> >
> >  >
> >  >
> >  > пн, 9 июл. 2018 г. в 22:29, Anton Vinogradov :>
> >  >
> >  > > 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 :>
> >  > >>
> >  > > > 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=
> buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E>
> >
> >
> >  > > >>
> >  > > >>
> >  > > > пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov :>
> >  > > >>
> >  > > > > Requirement of green TC 

Re: Ticket review checklist

[Vladimir Ozerov]

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  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  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  ···>
>  >
>  > 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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E>
>
>
>  >
>  >
>  > пн, 9 июл. 2018 г. в 22:29, Anton Vinogradov :>
>  >
>  > > 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 :>
>  > >>
>  > > > 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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E>
>
>
>  > > >>
>  > > >>
>  > > > пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov :>
>  > > >>
>  > > > > 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 

Re: Ticket review checklist

[ipavlukhin]

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  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  ···>
>
> 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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E> 


>
>
> пн, 9 июл. 2018 г. в 22:29, Anton Vinogradov :>
>
> > 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 :>
> >>
> > > 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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E> 


> > >>
> > >>
> > > пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov :>
> > >>
> > > > 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 :>
> > > >>
> > > > > 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 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 

Re: Ticket review checklist

[Dmitry Pavlov]

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  ···

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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E


пн, 9 июл. 2018 г. в 22:29, Anton Vinogradov :

> 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 :
>
> > 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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E
> >
> >
> > пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov :
> >
> > > 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 :
> > >
> > > > 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 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  >:
> > > > >
> > > > > > 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 

Re: Ticket review checklist

[Anton Vinogradov]

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 :

> 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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E
>
>
> пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov :
>
> > 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 :
> >
> > > 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 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 :
> > > >
> > > > > 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 

Re: Ticket review checklist

[Maxim Muzafarov]

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=buildTypeHistoryList_IgniteTests24Java8=%3Cdefault%3E


пн, 4 июн. 2018 г. в 15:56, Dmitry Pavlov :

> 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 :
>
> > 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 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 :
> > >
> > > > 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  >
> > > > 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  >:
> > > > >
> > > > > > 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
> > > 

Re: Ticket review checklist

[Dmitry Pavlov]

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 :

> 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 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 :
> >
> > > 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 
> > > 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 :
> > > >
> > > > > 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 :
> > > > > >
> > > > > > > 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' 

Re: Ticket review checklist

[Vladimir Ozerov]

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 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 :
>
> > 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 
> > 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 :
> > >
> > > > 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 :
> > > > >
> > > > > > 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
> > > > > > >
> > > > > > 

Re: Ticket review checklist

[Dmitry Pavlov]

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 :

> 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 
> 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 :
> >
> > > 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 
> > > 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 :
> > > >
> > > > > 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.
> > > > 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
> 

Re: Ticket review checklist

[Vladimir Ozerov]

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  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 :
>
> > 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 
> > 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 :
> > >
> > > > 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.
> > > 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  >:
> > > > >>
> > > > >>> 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 

Re: Ticket review checklist

[Dmitry Pavlov]

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 :

> 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 
> 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 :
> >
> > > 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.
> > 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 :
> > > >>
> > > >>> 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  >:
> > > >
> > > >> Andrey, Anton, Alex
> > > >>
> > > >> Agree, *SHOULD* is more appropriate here.
> > > >>
> > > >> Please see latest version below. Does anyone want to add or
> change
> > > >> 

Re: Ticket review checklist

[Vladimir Ozerov]

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 
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 :
>
> > 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.
> 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 
> > > 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 :
> > >>
> > >>> 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 :
> > >
> > >> 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 change it at any time if needed.
> > >>
> > >> 1) API
> > >> 1.1) API compatibility *MUST* be maintained between minor
> releases.
> > >> Do
> > > not
> > >> remove existing methods or change their signatures, deprecate them
> > > instead
> > >> 1.2) Default behaviour "SHOULD NOT* be changed between minor
> > >> releases,
> > >> unless absolutely needed. If change is made, it *MUST* be
> described
> > >> in
> > >> "Migration Guide"
> > >> 1.3) New operation *MUST* be well-documented in code (javadoc,
> > > dotnetdoc):
> > >> documentation 

Re: Ticket review checklist

[Dmitry Pavlov]

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 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. 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 
> > 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 :
> >>
> >>> 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  >
> >>> 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 :
> >
> >> 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 change it at any time if needed.
> >>
> >> 1) API
> >> 1.1) API compatibility *MUST* be maintained between minor releases.
> >> Do
> > not
> >> remove existing methods or change their signatures, deprecate them
> > instead
> >> 1.2) Default behaviour "SHOULD NOT* be changed between minor
> >> releases,
> >> unless absolutely needed. If change is made, it *MUST* be described
> >> in
> >> "Migration Guide"
> >> 1.3) New operation *MUST* be well-documented in code (javadoc,
> > dotnetdoc):
> >> documentation must contain method's purpose, description of
> >> parameters
> > and
> >> how their values affect the outcome, description of return value and
> > it's
> >> default, behavior in negative cases, interaction with other
> >> operations
> > and
> >> components
> >> 1.4) API parity between Java and .NET platforms *SHOULD* be
> >> maintained
> > when
> >> operation makes sense on both platforms. If method cannot be
> > implemented in
> >> a platform immediately, new JIRA 

Re: Ticket review checklist

[Dmitry Pavlov]

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
failures should be checked.

чт, 24 мая 2018 г. в 15:04, Vladimir Ozerov :

> 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 
> 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 :
> >
> > > 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  >
> > > 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 :
> > > >>
> > > >> > 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 change it at any time if needed.
> > > >> >
> > > >> > 1) API
> > > >> > 1.1) API compatibility *MUST* be maintained between minor
> releases.
> > Do
> > > >> not
> > > >> > remove existing methods or change their signatures, deprecate them
> > > >> instead
> > > >> > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> > releases,
> > > >> > unless absolutely needed. If change is made, it *MUST* be
> described
> > in
> > > >> > "Migration Guide"
> > > >> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > > >> dotnetdoc):
> > > >> > documentation must contain method's purpose, description of
> > parameters
> > > >> and
> > > >> > how their values affect the outcome, description of return value
> and
> > > >> it's
> > > >> > default, behavior in negative cases, interaction with other
> > operations
> > > >> and
> > > >> > components
> > > >> > 1.4) API parity between Java and .NET platforms *SHOULD* be
> > maintained
> > > >> when
> > > >> > operation makes sense on both platforms. If method cannot be
> > > >> implemented in
> > > >> > a platform immediately, new JIRA ticket *MUST* be 

Re: Ticket review checklist

[Petr Ivanov]

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. 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 
> 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 :
>> 
>>> 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 
>>> 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 :
> 
>> 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 change it at any time if needed.
>> 
>> 1) API
>> 1.1) API compatibility *MUST* be maintained between minor releases.
>> Do
> not
>> remove existing methods or change their signatures, deprecate them
> instead
>> 1.2) Default behaviour "SHOULD NOT* be changed between minor
>> releases,
>> unless absolutely needed. If change is made, it *MUST* be described
>> in
>> "Migration Guide"
>> 1.3) New operation *MUST* be well-documented in code (javadoc,
> dotnetdoc):
>> documentation must contain method's purpose, description of
>> parameters
> and
>> how their values affect the outcome, description of return value and
> it's
>> default, behavior in negative cases, interaction with other
>> operations
> and
>> components
>> 1.4) API parity between Java and .NET platforms *SHOULD* be
>> maintained
> when
>> operation makes sense on both platforms. If method cannot be
> implemented in
>> a platform immediately, new JIRA ticket *MUST* be created and linked
>>> to
>> current ticket
>> 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
>>> maintained
>> when operation makes sense on several clients. If method cannot be
>> implemented in a client immediately, new JIRA ticket *MUST* be
>> created
> and
>> linked to current ticket
>> 1.6) All exceptions thrown to a user **SHOULD** have explanation how
>>> to
>> resolve, workaround or debug an error
>> 
>> 2) 

Re: Ticket review checklist

[Vladimir Ozerov]

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 
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 :
>
> > 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 
> > 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 :
> > >>
> > >> > 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 change it at any time if needed.
> > >> >
> > >> > 1) API
> > >> > 1.1) API compatibility *MUST* be maintained between minor releases.
> Do
> > >> not
> > >> > remove existing methods or change their signatures, deprecate them
> > >> instead
> > >> > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> releases,
> > >> > unless absolutely needed. If change is made, it *MUST* be described
> in
> > >> > "Migration Guide"
> > >> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > >> dotnetdoc):
> > >> > documentation must contain method's purpose, description of
> parameters
> > >> and
> > >> > how their values affect the outcome, description of return value and
> > >> it's
> > >> > default, behavior in negative cases, interaction with other
> operations
> > >> and
> > >> > components
> > >> > 1.4) API parity between Java and .NET platforms *SHOULD* be
> maintained
> > >> when
> > >> > operation makes sense on both platforms. If method cannot be
> > >> implemented in
> > >> > a platform immediately, new JIRA ticket *MUST* be created and linked
> > to
> > >> > current ticket
> > >> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> > maintained
> > >> > when operation makes sense on several clients. If method cannot be
> > >> > implemented in a client immediately, new JIRA ticket *MUST* be
> created
> > >> and
> > >> > linked to current ticket
> > >> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how
> > to
> > >> > resolve, workaround or debug an error
> > >> >
> > >> > 2) Compatibility
> > >> > 2.1) Persistence backward compatibility *MUST* be maintained between
> > >> minor
> > >> > releases. It should be possible to start newer version on data 

Re: Ticket review checklist

[Dmitry Pavlov]

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 :

> 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 
> 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 :
> >>
> >> > 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 change it at any time if needed.
> >> >
> >> > 1) API
> >> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> >> not
> >> > remove existing methods or change their signatures, deprecate them
> >> instead
> >> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> >> > unless absolutely needed. If change is made, it *MUST* be described in
> >> > "Migration Guide"
> >> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> >> dotnetdoc):
> >> > documentation must contain method's purpose, description of parameters
> >> and
> >> > how their values affect the outcome, description of return value and
> >> it's
> >> > default, behavior in negative cases, interaction with other operations
> >> and
> >> > components
> >> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> >> when
> >> > operation makes sense on both platforms. If method cannot be
> >> implemented in
> >> > a platform immediately, new JIRA ticket *MUST* be created and linked
> to
> >> > current ticket
> >> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> maintained
> >> > when operation makes sense on several clients. If method cannot be
> >> > implemented in a client immediately, new JIRA ticket *MUST* be created
> >> and
> >> > linked to current ticket
> >> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how
> to
> >> > resolve, workaround or debug an error
> >> >
> >> > 2) Compatibility
> >> > 2.1) Persistence backward compatibility *MUST* be maintained between
> >> minor
> >> > releases. It should be possible to start newer version on data files
> >> > created by the previous version
> >> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> >> maintained
> >> > between two consecutive minor releases. If compatibility cannot be
> >> > maintained it *MUST* be described in "Migration Guide"
> >> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> >> > maintained between two consecutive minor releases. If compatibility
> >> cannot
> >> > be maintained it *MUST* be described in "Migration Guide"
> >> >
> >> > 3) Tests
> >> > 3.1) New functionality *MUST* be covered with 

Re: Ticket review checklist

[Vladimir Ozerov]

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 allow just one test failure, it would be more failures after some
> time.
>
> Sincererly,
> Dmitriy Pavlov
>
> ср, 23 мая 2018 г. в 17:02, Vladimir Ozerov :
>
> > 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 
> > 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 :
> > >>
> > >> > 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 change it at any time if needed.
> > >> >
> > >> > 1) API
> > >> > 1.1) API compatibility *MUST* be maintained between minor releases.
> Do
> > >> not
> > >> > remove existing methods or change their signatures, deprecate them
> > >> instead
> > >> > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> releases,
> > >> > unless absolutely needed. If change is made, it *MUST* be described
> in
> > >> > "Migration Guide"
> > >> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > >> dotnetdoc):
> > >> > documentation must contain method's purpose, description of
> parameters
> > >> and
> > >> > how their values affect the outcome, description of return value and
> > >> it's
> > >> > default, behavior in negative cases, interaction with other
> operations
> > >> and
> > >> > components
> > >> > 1.4) API parity between Java and .NET platforms *SHOULD* be
> maintained
> > >> when
> > >> > operation makes sense on both platforms. If method cannot be
> > >> implemented in
> > >> > a platform immediately, new JIRA ticket *MUST* be created and linked
> > to
> > >> > current ticket
> > >> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> > maintained
> > >> > when operation makes sense on several clients. If method cannot be
> > >> > implemented in a client immediately, new JIRA ticket *MUST* be
> created
> > >> and
> > >> > linked to current ticket
> > >> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how
> > to
> > >> > resolve, workaround or debug an error
> > >> >
> > >> > 2) Compatibility
> > >> > 2.1) Persistence backward compatibility *MUST* be maintained between
> > >> minor
> > >> > releases. It should be possible to start newer version on data files
> > >> > created by the previous version
> > >> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> > >> maintained
> > >> > between two consecutive minor releases. If compatibility cannot be
> > >> > maintained it *MUST* be described in "Migration Guide"
> > >> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* 

Re: Ticket review checklist

[Dmitry Pavlov]

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 :

> 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 
> 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 :
> >>
> >> > 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 change it at any time if needed.
> >> >
> >> > 1) API
> >> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> >> not
> >> > remove existing methods or change their signatures, deprecate them
> >> instead
> >> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> >> > unless absolutely needed. If change is made, it *MUST* be described in
> >> > "Migration Guide"
> >> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> >> dotnetdoc):
> >> > documentation must contain method's purpose, description of parameters
> >> and
> >> > how their values affect the outcome, description of return value and
> >> it's
> >> > default, behavior in negative cases, interaction with other operations
> >> and
> >> > components
> >> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> >> when
> >> > operation makes sense on both platforms. If method cannot be
> >> implemented in
> >> > a platform immediately, new JIRA ticket *MUST* be created and linked
> to
> >> > current ticket
> >> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> maintained
> >> > when operation makes sense on several clients. If method cannot be
> >> > implemented in a client immediately, new JIRA ticket *MUST* be created
> >> and
> >> > linked to current ticket
> >> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how
> to
> >> > resolve, workaround or debug an error
> >> >
> >> > 2) Compatibility
> >> > 2.1) Persistence backward compatibility *MUST* be maintained between
> >> minor
> >> > releases. It should be possible to start newer version on data files
> >> > created by the previous version
> >> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> >> maintained
> >> > between two consecutive minor releases. If compatibility cannot be
> >> > maintained it *MUST* be described in "Migration Guide"
> >> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> >> > maintained between two consecutive minor releases. If compatibility
> >> cannot
> >> > be maintained it *MUST* be described in "Migration Guide"
> >> >
> >> > 3) Tests
> >> > 3.1) New functionality *MUST* be covered with unit tests for both
> >> positive
> >> > and negative use cases
> >> > 3.2) All test suites *MUST* be run before merge to master..There
> *MUST*
> >> be
> >> > no new test failures

Re: Ticket review checklist

[Vladimir Ozerov]

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 
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 :
>>
>> > 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 change it at any time if needed.
>> >
>> > 1) API
>> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
>> not
>> > remove existing methods or change their signatures, deprecate them
>> instead
>> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
>> > unless absolutely needed. If change is made, it *MUST* be described in
>> > "Migration Guide"
>> > 1.3) New operation *MUST* be well-documented in code (javadoc,
>> dotnetdoc):
>> > documentation must contain method's purpose, description of parameters
>> and
>> > how their values affect the outcome, description of return value and
>> it's
>> > default, behavior in negative cases, interaction with other operations
>> and
>> > components
>> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
>> when
>> > operation makes sense on both platforms. If method cannot be
>> implemented in
>> > a platform immediately, new JIRA ticket *MUST* be created and linked to
>> > current ticket
>> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
>> > when operation makes sense on several clients. If method cannot be
>> > implemented in a client immediately, new JIRA ticket *MUST* be created
>> and
>> > linked to current ticket
>> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how to
>> > resolve, workaround or debug an error
>> >
>> > 2) Compatibility
>> > 2.1) Persistence backward compatibility *MUST* be maintained between
>> minor
>> > releases. It should be possible to start newer version on data files
>> > created by the previous version
>> > 2.2) Thin client forward and backward compatibility *SHOULD* be
>> maintained
>> > between two consecutive minor releases. If compatibility cannot be
>> > maintained it *MUST* be described in "Migration Guide"
>> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
>> > maintained between two consecutive minor releases. If compatibility
>> cannot
>> > be maintained it *MUST* be described in "Migration Guide"
>> >
>> > 3) Tests
>> > 3.1) New functionality *MUST* be covered with unit tests for both
>> positive
>> > and negative use cases
>> > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
>> be
>> > no new test failures
>> >
>> > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
>> >
>> > Vladimir.
>> >
>> >
>> > On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov 
>> > wrote:
>> >
>> > > 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 

Re: Ticket review checklist

[Vladimir Ozerov]

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 
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 :
>
> > 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 change it at any time if needed.
> >
> > 1) API
> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> not
> > remove existing methods or change their signatures, deprecate them
> instead
> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> > unless absolutely needed. If change is made, it *MUST* be described in
> > "Migration Guide"
> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> dotnetdoc):
> > documentation must contain method's purpose, description of parameters
> and
> > how their values affect the outcome, description of return value and it's
> > default, behavior in negative cases, interaction with other operations
> and
> > components
> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> when
> > operation makes sense on both platforms. If method cannot be implemented
> in
> > a platform immediately, new JIRA ticket *MUST* be created and linked to
> > current ticket
> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> > when operation makes sense on several clients. If method cannot be
> > implemented in a client immediately, new JIRA ticket *MUST* be created
> and
> > linked to current ticket
> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how to
> > resolve, workaround or debug an error
> >
> > 2) Compatibility
> > 2.1) Persistence backward compatibility *MUST* be maintained between
> minor
> > releases. It should be possible to start newer version on data files
> > created by the previous version
> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> maintained
> > between two consecutive minor releases. If compatibility cannot be
> > maintained it *MUST* be described in "Migration Guide"
> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > maintained between two consecutive minor releases. If compatibility
> cannot
> > be maintained it *MUST* be described in "Migration Guide"
> >
> > 3) Tests
> > 3.1) New functionality *MUST* be covered with unit tests for both
> positive
> > and negative use cases
> > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
> be
> > no new test failures
> >
> > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> >
> > Vladimir.
> >
> >
> > On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov 
> > wrote:
> >
> > > 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 exceptions thrown to a user *SHOULD* have explanation how to
> > resolve
> > > if
> > > > possible.
> > > > ?
> > > >
> > > > вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov :
> > > >
> > > > > 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
> 

Re: Ticket review checklist

[Dmitry Pavlov]

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 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 brilliant
> example
> >> how product should not behave.
> Dmitry, could you please provide correct message for this case?
>
>
> чт, 10 мая 2018 г. в 16:23, Ilya Kasnacheev :
>
> > 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 Pavlov :
> >
> > > 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 defenetely reduce number of messages to user
> > list.
> > >
> > > Sincerely,
> > > Dmitriy Pavlov
> > >
> > >
> > > чт, 10 мая 2018 г. в 15:50, Ilya Kasnacheev  >:
> > >
> > > > 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 :
> > > >
> > > > > 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 change it at any time if needed.
> > > > >
> > > > > 1) API
> > > > > 1.1) API compatibility *MUST* be maintained between minor releases.
> > Do
> > > > not
> > > > > remove existing methods or change their signatures, deprecate them
> > > > instead
> > > > > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> > releases,
> > > > > unless absolutely needed. If change is made, it *MUST* be described
> > in
> > > > > "Migration Guide"
> > > > > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > > > dotnetdoc):
> > > > > documentation must contain method's purpose, description of
> > parameters
> > > > and
> > > > > how their values affect the outcome, description of return value
> and
> > > it's
> > > > > default, behavior in negative cases, interaction with other
> > operations
> > > > and
> > > > > components
> > > > > 1.4) API parity between Java and .NET platforms *SHOULD* be
> > maintained
> > > > when
> > > > > operation makes sense on both platforms. If method cannot be
> > > implemented
> > > > in
> > > > > a platform immediately, new JIRA ticket *MUST* be created and
> linked
> > to
> > > > > current ticket
> > > > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> > > maintained
> > > > > when operation makes sense on several clients. If method cannot be
> > > > > implemented in a client immediately, new JIRA ticket *MUST* be
> > created
> > > > and
> > > > > linked to current ticket
> > > > > 1.6) All exceptions thrown to a user **SHOULD** have explanation
> how
> > to
> > > > > resolve, workaround or debug an error
> > > > >
> > > > > 2) Compatibility
> > > > > 2.1) Persistence backward compatibility *MUST* be maintained
> between
> > > > minor
> > > > > releases. It should be possible to start newer version on data
> files
> > > > > created by the previous version
> > > > > 2.2) Thin client forward 

Re: Ticket review checklist

[Anton Vinogradov]

 >> 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 brilliant
example
>> how product should not behave.
Dmitry, could you please provide correct message for this case?


чт, 10 мая 2018 г. в 16:23, Ilya Kasnacheev :

> 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 Pavlov :
>
> > 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 defenetely reduce number of messages to user
> list.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> >
> > чт, 10 мая 2018 г. в 15:50, Ilya Kasnacheev :
> >
> > > 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 :
> > >
> > > > 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 change it at any time if needed.
> > > >
> > > > 1) API
> > > > 1.1) API compatibility *MUST* be maintained between minor releases.
> Do
> > > not
> > > > remove existing methods or change their signatures, deprecate them
> > > instead
> > > > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> releases,
> > > > unless absolutely needed. If change is made, it *MUST* be described
> in
> > > > "Migration Guide"
> > > > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > > dotnetdoc):
> > > > documentation must contain method's purpose, description of
> parameters
> > > and
> > > > how their values affect the outcome, description of return value and
> > it's
> > > > default, behavior in negative cases, interaction with other
> operations
> > > and
> > > > components
> > > > 1.4) API parity between Java and .NET platforms *SHOULD* be
> maintained
> > > when
> > > > operation makes sense on both platforms. If method cannot be
> > implemented
> > > in
> > > > a platform immediately, new JIRA ticket *MUST* be created and linked
> to
> > > > current ticket
> > > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> > maintained
> > > > when operation makes sense on several clients. If method cannot be
> > > > implemented in a client immediately, new JIRA ticket *MUST* be
> created
> > > and
> > > > linked to current ticket
> > > > 1.6) All exceptions thrown to a user **SHOULD** have explanation how
> to
> > > > resolve, workaround or debug an error
> > > >
> > > > 2) Compatibility
> > > > 2.1) Persistence backward compatibility *MUST* be maintained between
> > > minor
> > > > releases. It should be possible to start newer version on data files
> > > > created by the previous version
> > > > 2.2) Thin client forward and backward compatibility *SHOULD* be
> > > maintained
> > > > between two consecutive minor releases. If compatibility cannot be
> > > > maintained it *MUST* be described in "Migration Guide"
> > > > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > > > maintained between two consecutive minor releases. If compatibility
> > > cannot
> > > > be maintained it *MUST* be described in "Migration Guide"
> > > >
> > > > 3) Tests
> > > > 3.1) New functionality *MUST* be covered with unit 

Re: Ticket review checklist

[Ilya Kasnacheev]

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 Pavlov :

> 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 defenetely reduce number of messages to user list.
>
> Sincerely,
> Dmitriy Pavlov
>
>
> чт, 10 мая 2018 г. в 15:50, Ilya Kasnacheev :
>
> > 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 :
> >
> > > 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 change it at any time if needed.
> > >
> > > 1) API
> > > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> > not
> > > remove existing methods or change their signatures, deprecate them
> > instead
> > > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> > > unless absolutely needed. If change is made, it *MUST* be described in
> > > "Migration Guide"
> > > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > dotnetdoc):
> > > documentation must contain method's purpose, description of parameters
> > and
> > > how their values affect the outcome, description of return value and
> it's
> > > default, behavior in negative cases, interaction with other operations
> > and
> > > components
> > > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> > when
> > > operation makes sense on both platforms. If method cannot be
> implemented
> > in
> > > a platform immediately, new JIRA ticket *MUST* be created and linked to
> > > current ticket
> > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> maintained
> > > when operation makes sense on several clients. If method cannot be
> > > implemented in a client immediately, new JIRA ticket *MUST* be created
> > and
> > > linked to current ticket
> > > 1.6) All exceptions thrown to a user **SHOULD** have explanation how to
> > > resolve, workaround or debug an error
> > >
> > > 2) Compatibility
> > > 2.1) Persistence backward compatibility *MUST* be maintained between
> > minor
> > > releases. It should be possible to start newer version on data files
> > > created by the previous version
> > > 2.2) Thin client forward and backward compatibility *SHOULD* be
> > maintained
> > > between two consecutive minor releases. If compatibility cannot be
> > > maintained it *MUST* be described in "Migration Guide"
> > > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > > maintained between two consecutive minor releases. If compatibility
> > cannot
> > > be maintained it *MUST* be described in "Migration Guide"
> > >
> > > 3) Tests
> > > 3.1) New functionality *MUST* be covered with unit tests for both
> > positive
> > > and negative use cases
> > > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
> > be
> > > no new test failures
> > >
> > > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> > >
> > > Vladimir.
> > >
> > >
> > > On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov 
> > > wrote:
> > >
> > > > 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
> > > > > 

Re: Ticket review checklist

[Dmitry Pavlov]

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 defenetely reduce number of messages to user list.

Sincerely,
Dmitriy Pavlov


чт, 10 мая 2018 г. в 15:50, Ilya Kasnacheev :

> 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 :
>
> > 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 change it at any time if needed.
> >
> > 1) API
> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> not
> > remove existing methods or change their signatures, deprecate them
> instead
> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> > unless absolutely needed. If change is made, it *MUST* be described in
> > "Migration Guide"
> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> dotnetdoc):
> > documentation must contain method's purpose, description of parameters
> and
> > how their values affect the outcome, description of return value and it's
> > default, behavior in negative cases, interaction with other operations
> and
> > components
> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> when
> > operation makes sense on both platforms. If method cannot be implemented
> in
> > a platform immediately, new JIRA ticket *MUST* be created and linked to
> > current ticket
> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> > when operation makes sense on several clients. If method cannot be
> > implemented in a client immediately, new JIRA ticket *MUST* be created
> and
> > linked to current ticket
> > 1.6) All exceptions thrown to a user **SHOULD** have explanation how to
> > resolve, workaround or debug an error
> >
> > 2) Compatibility
> > 2.1) Persistence backward compatibility *MUST* be maintained between
> minor
> > releases. It should be possible to start newer version on data files
> > created by the previous version
> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> maintained
> > between two consecutive minor releases. If compatibility cannot be
> > maintained it *MUST* be described in "Migration Guide"
> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > maintained between two consecutive minor releases. If compatibility
> cannot
> > be maintained it *MUST* be described in "Migration Guide"
> >
> > 3) Tests
> > 3.1) New functionality *MUST* be covered with unit tests for both
> positive
> > and negative use cases
> > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
> be
> > no new test failures
> >
> > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> >
> > Vladimir.
> >
> >
> > On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov 
> > wrote:
> >
> > > 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 exceptions thrown to a user *SHOULD* have explanation how to
> > resolve
> > > if
> > > > possible.
> > > > ?
> > > >
> > > > вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov :
> > > >
> > > > > 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 

Re: Ticket review checklist

[Ilya Kasnacheev]

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 :

> 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 change it at any time if needed.
>
> 1) API
> 1.1) API compatibility *MUST* be maintained between minor releases. Do not
> remove existing methods or change their signatures, deprecate them instead
> 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> unless absolutely needed. If change is made, it *MUST* be described in
> "Migration Guide"
> 1.3) New operation *MUST* be well-documented in code (javadoc, dotnetdoc):
> documentation must contain method's purpose, description of parameters and
> how their values affect the outcome, description of return value and it's
> default, behavior in negative cases, interaction with other operations and
> components
> 1.4) API parity between Java and .NET platforms *SHOULD* be maintained when
> operation makes sense on both platforms. If method cannot be implemented in
> a platform immediately, new JIRA ticket *MUST* be created and linked to
> current ticket
> 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> when operation makes sense on several clients. If method cannot be
> implemented in a client immediately, new JIRA ticket *MUST* be created and
> linked to current ticket
> 1.6) All exceptions thrown to a user **SHOULD** have explanation how to
> resolve, workaround or debug an error
>
> 2) Compatibility
> 2.1) Persistence backward compatibility *MUST* be maintained between minor
> releases. It should be possible to start newer version on data files
> created by the previous version
> 2.2) Thin client forward and backward compatibility *SHOULD* be maintained
> between two consecutive minor releases. If compatibility cannot be
> maintained it *MUST* be described in "Migration Guide"
> 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> maintained between two consecutive minor releases. If compatibility cannot
> be maintained it *MUST* be described in "Migration Guide"
>
> 3) Tests
> 3.1) New functionality *MUST* be covered with unit tests for both positive
> and negative use cases
> 3.2) All test suites *MUST* be run before merge to master..There *MUST* be
> no new test failures
>
> 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
>
> Vladimir.
>
>
> On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov 
> wrote:
>
> > 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 exceptions thrown to a user *SHOULD* have explanation how to
> resolve
> > if
> > > possible.
> > > ?
> > >
> > > вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov :
> > >
> > > > 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 :
> > > >
> > > > > 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 create
> duplicates.
> > > > >
> > > > > вт, 8 мая 2018 г. в 10:19, Александр Меньшиков <
> sharple...@gmail.com
> > >:
> > > > >
> > > > > > Vladimir, the 3.1 is a bit unclear for me. Which code coverage is
> > > > > > acceptable? Now it 

Re: Ticket review checklist

[Vladimir Ozerov]

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 change it at any time if needed.

1) API
1.1) API compatibility *MUST* be maintained between minor releases. Do not
remove existing methods or change their signatures, deprecate them instead
1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
unless absolutely needed. If change is made, it *MUST* be described in
"Migration Guide"
1.3) New operation *MUST* be well-documented in code (javadoc, dotnetdoc):
documentation must contain method's purpose, description of parameters and
how their values affect the outcome, description of return value and it's
default, behavior in negative cases, interaction with other operations and
components
1.4) API parity between Java and .NET platforms *SHOULD* be maintained when
operation makes sense on both platforms. If method cannot be implemented in
a platform immediately, new JIRA ticket *MUST* be created and linked to
current ticket
1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
when operation makes sense on several clients. If method cannot be
implemented in a client immediately, new JIRA ticket *MUST* be created and
linked to current ticket
1.6) All exceptions thrown to a user **SHOULD** have explanation how to
resolve, workaround or debug an error

2) Compatibility
2.1) Persistence backward compatibility *MUST* be maintained between minor
releases. It should be possible to start newer version on data files
created by the previous version
2.2) Thin client forward and backward compatibility *SHOULD* be maintained
between two consecutive minor releases. If compatibility cannot be
maintained it *MUST* be described in "Migration Guide"
2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
maintained between two consecutive minor releases. If compatibility cannot
be maintained it *MUST* be described in "Migration Guide"

3) Tests
3.1) New functionality *MUST* be covered with unit tests for both positive
and negative use cases
3.2) All test suites *MUST* be run before merge to master..There *MUST* be
no new test failures

4) Code style *MUST* be followed as per Ignite's Coding Guidelines

Vladimir.


On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov  wrote:

> 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 exceptions thrown to a user *SHOULD* have explanation how to resolve
> if
> > possible.
> > ?
> >
> > вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov :
> >
> > > 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 :
> > >
> > > > 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 create duplicates.
> > > >
> > > > вт, 8 мая 2018 г. в 10:19, Александр Меньшиков  >:
> > > >
> > > > > 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 <
> dsetrak...@apache.org
> > >:
> > > > >
> > > > > > Is this list on the Wiki?
> > > > > >
> > > > > > On Mon, May 7, 2018 at 7:26 AM, Vladimir Ozerov <
> > > voze...@gridgain.com>
> > > > > > 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 only strict rules, but *nice to have*
> points
> > > > here
> > > > > as
> > > > > > > well with help of *MUST*, *SHOULD* and *MAY* words as per
> RFC2119
> > > > [1].
> > > > > So
> > > > > > > please feel free to suggest optional items as well.
> > > > > > >
> > > > > > > 1) API
> > > > > > > 1.1) API compatibility *MUST* be maintained between minor
> > releases.
> > > > Do

Re: Ticket review checklist

[Vladimir Ozerov]

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 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 RFC2119 [1]. So
> > please feel free to suggest optional items as well.
> >
> > 1) API
> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> not
> > remove existing methods or change their signatures, deprecate them
> instead
> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> > unless absolutely needed. If change is made, it *MUST* be described in
> > "Migration Guide"
> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> dotnetdoc):
> > documentation must contain method's purpose, description of parameters
> and
> > how their values affect the outcome, description of return value and it's
> > default, behavior in negative cases, interaction with other operations
> and
> > components
> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> when
> > operation makes sense on both platforms. If method cannot be implemented
> in
> > a platform immediately, new JIRA ticket *MUST* be created and linked to
> > current ticket
> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> > when operation makes sense on several clients. If method cannot be
> > implemented in a client immediately, new JIRA ticket *MUST* be created
> and
> > linked to current ticket
> > 1.6) All exceptions thrown to a user *MUST* have explanation how to
> > resolve, workaround or debug an error
> >
> > 2) Compatibility
> > 2.1) Persistence backward compatibility *MUST* be maintained between
> minor
> > releases. It should be possible to start newer version on data files
> > created by the previous version
> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> maintained
> > between two consecutive minor releases. If compatibility cannot be
> > maintained it *MUST* be described in "Migration Guide"
> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > maintained between two consecutive minor releases. If compatibility
> cannot
> > be maintained it *MUST* be described in "Migration Guide"
> >
> > 3) Tests
> > 3.1) New functionality *MUST* be covered with unit tests for both
> positive
> > and negative use cases
> > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
> be
> > no new test failures
> >
> > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> >
> > Vladimir.
> >
> > [1] https://www.ietf.org/rfc/rfc2119.txt
> >
> > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov 
> > wrote:
> >
> > > 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
> > >> of practice. Approving of standalone refactorings instead contradicts
> > with
> > >> principle don't touch if it works. So I still like idea of co-located
> > >> changes improving code, javadocs, style, etc.
> > >>
> > >> But let's not argue about this point now, let's summarize the
> undisputed
> > >> points and add it to the wiki. Vladimir, would you please do it?
> > >>
> > >>
> > >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
> > >>
> > >> > Igniters,
> > >> >
> > >> > I agree with Vova.
> > >> >
> > >> > Don't fix if it works!
> > >> >
> > >> > If you 100% sure then it a useful addition to the product - just
> make
> > a
> > >> > separate ticket.
> > >> >
> > >> > В Ср, 25/04/2018 в 11:44 +0300, 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 org.jsr166 package in
> > >> favor
> > >> > of
> > >> > > Java 8 classes. Innocent change. Result - broken storage. Another
> > >> problem
> > >> > > is conflicts. It is not uncommon to have long-lived branches which
> > we
> > >> > need
> > >> > > to merge with master over and over again. And a lot of
> refactorings
> > >> cause
> > >> > > conflicts. It is much easier to resolve them if you know that
> logic
> > >> was
> > >> > not
> > >> > > affected as opposed to cases when you need to resolve both 

Re: Ticket review checklist

[Vladimir Ozerov]

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 checklist implementer doesn't know about the requirement and does
not address it. With checklist it is addressed. However, at the moment we
do not have a framework to check clients compatibiltiy and it is rather
hard to implement. So ideally implementer could demonstrate tests, but also
he can just state "I tested it manually", which is also acceptable. This is
where trust comes into play.

On Mon, May 7, 2018 at 6:45 PM, Anton Vinogradov  wrote:

> 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 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 RFC2119 [1]. So
> > please feel free to suggest optional items as well.
> >
> > 1) API
> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> not
> > remove existing methods or change their signatures, deprecate them
> instead
> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> > unless absolutely needed. If change is made, it *MUST* be described in
> > "Migration Guide"
> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> dotnetdoc):
> > documentation must contain method's purpose, description of parameters
> and
> > how their values affect the outcome, description of return value and it's
> > default, behavior in negative cases, interaction with other operations
> and
> > components
> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> when
> > operation makes sense on both platforms. If method cannot be implemented
> in
> > a platform immediately, new JIRA ticket *MUST* be created and linked to
> > current ticket
> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> > when operation makes sense on several clients. If method cannot be
> > implemented in a client immediately, new JIRA ticket *MUST* be created
> and
> > linked to current ticket
> > 1.6) All exceptions thrown to a user *MUST* have explanation how to
> > resolve, workaround or debug an error
> >
> > 2) Compatibility
> > 2.1) Persistence backward compatibility *MUST* be maintained between
> minor
> > releases. It should be possible to start newer version on data files
> > created by the previous version
> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> maintained
> > between two consecutive minor releases. If compatibility cannot be
> > maintained it *MUST* be described in "Migration Guide"
> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > maintained between two consecutive minor releases. If compatibility
> cannot
> > be maintained it *MUST* be described in "Migration Guide"
> >
> > 3) Tests
> > 3.1) New functionality *MUST* be covered with unit tests for both
> positive
> > and negative use cases
> > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
> be
> > no new test failures
> >
> > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> >
> > Vladimir.
> >
> > [1] https://www.ietf.org/rfc/rfc2119.txt
> >
> > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov 
> > wrote:
> >
> > > 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
> > >> of practice. Approving of standalone refactorings instead contradicts
> > with
> > >> principle don't touch if it works. So I still like idea of co-located
> > >> changes improving code, javadocs, style, etc.
> > >>
> > >> But let's not argue about this point now, let's summarize the
> undisputed
> > >> points and add it to the wiki. Vladimir, would you please do it?
> > >>
> > >>
> > >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
> > >>
> > >> > Igniters,
> > >> >
> > >> > I agree with Vova.
> > >> >
> > >> > Don't fix if it works!
> > >> >
> > >> > If you 100% sure then it a useful addition to the product - just
> make
> > a
> > >> > separate ticket.
> > >> >
> > >> > В Ср, 25/04/2018 в 11:44 +0300, Vladimir Ozerov пишет:
> > >> > > Guys,
> > >> > >
> > >> > 

Re: Ticket review checklist

[Andrey Kuznetsov]

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 exceptions thrown to a user *SHOULD* have explanation how to resolve if
> possible.
> ?
>
> вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov :
>
> > 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 :
> >
> > > 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 create duplicates.
> > >
> > > вт, 8 мая 2018 г. в 10:19, Александр Меньшиков :
> > >
> > > > 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 AM, Vladimir Ozerov <
> > voze...@gridgain.com>
> > > > > 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 only strict rules, but *nice to have* points
> > > here
> > > > as
> > > > > > well with help of *MUST*, *SHOULD* and *MAY* words as per RFC2119
> > > [1].
> > > > So
> > > > > > please feel free to suggest optional items as well.
> > > > > >
> > > > > > 1) API
> > > > > > 1.1) API compatibility *MUST* be maintained between minor
> releases.
> > > Do
> > > > > not
> > > > > > remove existing methods or change their signatures, deprecate
> them
> > > > > instead
> > > > > > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> > > releases,
> > > > > > unless absolutely needed. If change is made, it *MUST* be
> described
> > > in
> > > > > > "Migration Guide"
> > > > > > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > > > > dotnetdoc):
> > > > > > documentation must contain method's purpose, description of
> > > parameters
> > > > > and
> > > > > > how their values affect the outcome, description of return value
> > and
> > > > it's
> > > > > > default, behavior in negative cases, interaction with other
> > > operations
> > > > > and
> > > > > > components
> > > > > > 1.4) API parity between Java and .NET platforms *SHOULD* be
> > > maintained
> > > > > when
> > > > > > operation makes sense on both platforms. If method cannot be
> > > > implemented
> > > > > in
> > > > > > a platform immediately, new JIRA ticket *MUST* be created and
> > linked
> > > to
> > > > > > current ticket
> > > > > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> > > > maintained
> > > > > > when operation makes sense on several clients. If method cannot
> be
> > > > > > implemented in a client immediately, new JIRA ticket *MUST* be
> > > created
> > > > > and
> > > > > > linked to current ticket
> > > > > > 1.6) All exceptions thrown to a user *MUST* have explanation how
> to
> > > > > > resolve, workaround or debug an error
> > > > > >
> > > > > > 2) Compatibility
> > > > > > 2.1) Persistence backward compatibility *MUST* be maintained
> > between
> > > > > minor
> > > > > > releases. It should be possible to start newer version on data
> > files
> > > > > > created by the previous version
> > > > > > 2.2) Thin client forward and backward compatibility *SHOULD* be
> > > > > maintained
> > > > > > between two consecutive minor releases. If compatibility cannot
> be
> > > > > > maintained it *MUST* be described in "Migration Guide"
> > > > > > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > > > > > maintained between two consecutive minor releases. If
> compatibility
> > > > > cannot
> > > > > > be maintained it *MUST* be described in "Migration Guide"
> > > > > >
> > > > > > 3) Tests
> > > > > > 3.1) New functionality *MUST* be covered with unit tests for both
> > > > > positive
> > > > > > and negative use cases
> > > > > > 3.2) All test suites *MUST* be run before merge to master..There
> > > *MUST*
> > > > > be
> > > > > > no new test failures
> > > > > >
> > > > > > 4) Code style *MUST* be followed as per Ignite's 

Re: Ticket review checklist

[Anton Vinogradov]

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 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 :
>
> > 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 create duplicates.
> >
> > вт, 8 мая 2018 г. в 10:19, Александр Меньшиков :
> >
> > > 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 AM, Vladimir Ozerov <
> voze...@gridgain.com>
> > > > 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 only strict rules, but *nice to have* points
> > here
> > > as
> > > > > well with help of *MUST*, *SHOULD* and *MAY* words as per RFC2119
> > [1].
> > > So
> > > > > please feel free to suggest optional items as well.
> > > > >
> > > > > 1) API
> > > > > 1.1) API compatibility *MUST* be maintained between minor releases.
> > Do
> > > > not
> > > > > remove existing methods or change their signatures, deprecate them
> > > > instead
> > > > > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> > releases,
> > > > > unless absolutely needed. If change is made, it *MUST* be described
> > in
> > > > > "Migration Guide"
> > > > > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > > > dotnetdoc):
> > > > > documentation must contain method's purpose, description of
> > parameters
> > > > and
> > > > > how their values affect the outcome, description of return value
> and
> > > it's
> > > > > default, behavior in negative cases, interaction with other
> > operations
> > > > and
> > > > > components
> > > > > 1.4) API parity between Java and .NET platforms *SHOULD* be
> > maintained
> > > > when
> > > > > operation makes sense on both platforms. If method cannot be
> > > implemented
> > > > in
> > > > > a platform immediately, new JIRA ticket *MUST* be created and
> linked
> > to
> > > > > current ticket
> > > > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> > > maintained
> > > > > when operation makes sense on several clients. If method cannot be
> > > > > implemented in a client immediately, new JIRA ticket *MUST* be
> > created
> > > > and
> > > > > linked to current ticket
> > > > > 1.6) All exceptions thrown to a user *MUST* have explanation how to
> > > > > resolve, workaround or debug an error
> > > > >
> > > > > 2) Compatibility
> > > > > 2.1) Persistence backward compatibility *MUST* be maintained
> between
> > > > minor
> > > > > releases. It should be possible to start newer version on data
> files
> > > > > created by the previous version
> > > > > 2.2) Thin client forward and backward compatibility *SHOULD* be
> > > > maintained
> > > > > between two consecutive minor releases. If compatibility cannot be
> > > > > maintained it *MUST* be described in "Migration Guide"
> > > > > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > > > > maintained between two consecutive minor releases. If compatibility
> > > > cannot
> > > > > be maintained it *MUST* be described in "Migration Guide"
> > > > >
> > > > > 3) Tests
> > > > > 3.1) New functionality *MUST* be covered with unit tests for both
> > > > positive
> > > > > and negative use cases
> > > > > 3.2) All test suites *MUST* be run before merge to master..There
> > *MUST*
> > > > be
> > > > > no new test failures
> > > > >
> > > > > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> > > > >
> > > > > Vladimir.
> > > > >
> > > > > [1] https://www.ietf.org/rfc/rfc2119.txt
> > > > >
> > > > > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov <
> > voze...@gridgain.com>
> > > > > wrote:
> > > > >
> > > > > > Hi Dmitry,
> > > > > >
> > > > > > Yes, I'll do that in the nearest days.
> > > > > >
> > > > > > On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <
> > > dpavlov@gmail.com>
> > > > > > wrote:
> 

Re: Ticket review checklist

[Andrey Kuznetsov]

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 :

> 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 create duplicates.
>
> вт, 8 мая 2018 г. в 10:19, Александр Меньшиков :
>
> > 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 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 only strict rules, but *nice to have* points
> here
> > as
> > > > well with help of *MUST*, *SHOULD* and *MAY* words as per RFC2119
> [1].
> > So
> > > > please feel free to suggest optional items as well.
> > > >
> > > > 1) API
> > > > 1.1) API compatibility *MUST* be maintained between minor releases.
> Do
> > > not
> > > > remove existing methods or change their signatures, deprecate them
> > > instead
> > > > 1.2) Default behaviour "SHOULD NOT* be changed between minor
> releases,
> > > > unless absolutely needed. If change is made, it *MUST* be described
> in
> > > > "Migration Guide"
> > > > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > > dotnetdoc):
> > > > documentation must contain method's purpose, description of
> parameters
> > > and
> > > > how their values affect the outcome, description of return value and
> > it's
> > > > default, behavior in negative cases, interaction with other
> operations
> > > and
> > > > components
> > > > 1.4) API parity between Java and .NET platforms *SHOULD* be
> maintained
> > > when
> > > > operation makes sense on both platforms. If method cannot be
> > implemented
> > > in
> > > > a platform immediately, new JIRA ticket *MUST* be created and linked
> to
> > > > current ticket
> > > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> > maintained
> > > > when operation makes sense on several clients. If method cannot be
> > > > implemented in a client immediately, new JIRA ticket *MUST* be
> created
> > > and
> > > > linked to current ticket
> > > > 1.6) All exceptions thrown to a user *MUST* have explanation how to
> > > > resolve, workaround or debug an error
> > > >
> > > > 2) Compatibility
> > > > 2.1) Persistence backward compatibility *MUST* be maintained between
> > > minor
> > > > releases. It should be possible to start newer version on data files
> > > > created by the previous version
> > > > 2.2) Thin client forward and backward compatibility *SHOULD* be
> > > maintained
> > > > between two consecutive minor releases. If compatibility cannot be
> > > > maintained it *MUST* be described in "Migration Guide"
> > > > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > > > maintained between two consecutive minor releases. If compatibility
> > > cannot
> > > > be maintained it *MUST* be described in "Migration Guide"
> > > >
> > > > 3) Tests
> > > > 3.1) New functionality *MUST* be covered with unit tests for both
> > > positive
> > > > and negative use cases
> > > > 3.2) All test suites *MUST* be run before merge to master..There
> *MUST*
> > > be
> > > > no new test failures
> > > >
> > > > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> > > >
> > > > Vladimir.
> > > >
> > > > [1] https://www.ietf.org/rfc/rfc2119.txt
> > > >
> > > > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov <
> voze...@gridgain.com>
> > > > wrote:
> > > >
> > > > > Hi Dmitry,
> > > > >
> > > > > Yes, I'll do that in the nearest days.
> > > > >
> > > > > On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <
> > dpavlov@gmail.com>
> > > > > 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
> > > > >> of practice. Approving of standalone refactorings instead
> > contradicts
> > > > with
> > > > >> principle don't touch if it works. So I still like idea of
> > co-located
> > > > >> changes improving code, javadocs, style, etc.
> > > > >>
> > > > >> But let's not argue about this point now, let's summarize 

Re: Ticket review checklist

[Anton Vinogradov]

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 create duplicates.

вт, 8 мая 2018 г. в 10:19, Александр Меньшиков :

> 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 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 only strict rules, but *nice to have* points here
> as
> > > well with help of *MUST*, *SHOULD* and *MAY* words as per RFC2119 [1].
> So
> > > please feel free to suggest optional items as well.
> > >
> > > 1) API
> > > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> > not
> > > remove existing methods or change their signatures, deprecate them
> > instead
> > > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> > > unless absolutely needed. If change is made, it *MUST* be described in
> > > "Migration Guide"
> > > 1.3) New operation *MUST* be well-documented in code (javadoc,
> > dotnetdoc):
> > > documentation must contain method's purpose, description of parameters
> > and
> > > how their values affect the outcome, description of return value and
> it's
> > > default, behavior in negative cases, interaction with other operations
> > and
> > > components
> > > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> > when
> > > operation makes sense on both platforms. If method cannot be
> implemented
> > in
> > > a platform immediately, new JIRA ticket *MUST* be created and linked to
> > > current ticket
> > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be
> maintained
> > > when operation makes sense on several clients. If method cannot be
> > > implemented in a client immediately, new JIRA ticket *MUST* be created
> > and
> > > linked to current ticket
> > > 1.6) All exceptions thrown to a user *MUST* have explanation how to
> > > resolve, workaround or debug an error
> > >
> > > 2) Compatibility
> > > 2.1) Persistence backward compatibility *MUST* be maintained between
> > minor
> > > releases. It should be possible to start newer version on data files
> > > created by the previous version
> > > 2.2) Thin client forward and backward compatibility *SHOULD* be
> > maintained
> > > between two consecutive minor releases. If compatibility cannot be
> > > maintained it *MUST* be described in "Migration Guide"
> > > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > > maintained between two consecutive minor releases. If compatibility
> > cannot
> > > be maintained it *MUST* be described in "Migration Guide"
> > >
> > > 3) Tests
> > > 3.1) New functionality *MUST* be covered with unit tests for both
> > positive
> > > and negative use cases
> > > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
> > be
> > > no new test failures
> > >
> > > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> > >
> > > Vladimir.
> > >
> > > [1] https://www.ietf.org/rfc/rfc2119.txt
> > >
> > > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov 
> > > wrote:
> > >
> > > > Hi Dmitry,
> > > >
> > > > Yes, I'll do that in the nearest days.
> > > >
> > > > On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <
> dpavlov@gmail.com>
> > > > 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
> > > >> of practice. Approving of standalone refactorings instead
> contradicts
> > > with
> > > >> principle don't touch if it works. So I still like idea of
> co-located
> > > >> changes improving code, javadocs, style, etc.
> > > >>
> > > >> But let's not argue about this point now, let's summarize the
> > undisputed
> > > >> points and add it to the wiki. Vladimir, would you please do it?
> > > >>
> > > >>
> > > >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
> > > >>
> > > >> > Igniters,
> > > >> >
> > > >> > I agree with Vova.
> > > >> >
> > > >> > Don't fix if it works!
> > > >> >
> > > >> > If you 100% sure then it a useful addition to the product - just
> > make
> > > a
> > > >> > separate ticket.
> > > >> >
> > > >> > В Ср, 25/04/2018 в 11:44 +0300, Vladimir Ozerov пишет:
> > > >> > > Guys,
> > > >> > >
> > > >> > > The problem with 

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 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 only strict rules, but *nice to have* points here as
> > well with help of *MUST*, *SHOULD* and *MAY* words as per RFC2119 [1]. So
> > please feel free to suggest optional items as well.
> >
> > 1) API
> > 1.1) API compatibility *MUST* be maintained between minor releases. Do
> not
> > remove existing methods or change their signatures, deprecate them
> instead
> > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> > unless absolutely needed. If change is made, it *MUST* be described in
> > "Migration Guide"
> > 1.3) New operation *MUST* be well-documented in code (javadoc,
> dotnetdoc):
> > documentation must contain method's purpose, description of parameters
> and
> > how their values affect the outcome, description of return value and it's
> > default, behavior in negative cases, interaction with other operations
> and
> > components
> > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained
> when
> > operation makes sense on both platforms. If method cannot be implemented
> in
> > a platform immediately, new JIRA ticket *MUST* be created and linked to
> > current ticket
> > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> > when operation makes sense on several clients. If method cannot be
> > implemented in a client immediately, new JIRA ticket *MUST* be created
> and
> > linked to current ticket
> > 1.6) All exceptions thrown to a user *MUST* have explanation how to
> > resolve, workaround or debug an error
> >
> > 2) Compatibility
> > 2.1) Persistence backward compatibility *MUST* be maintained between
> minor
> > releases. It should be possible to start newer version on data files
> > created by the previous version
> > 2.2) Thin client forward and backward compatibility *SHOULD* be
> maintained
> > between two consecutive minor releases. If compatibility cannot be
> > maintained it *MUST* be described in "Migration Guide"
> > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> > maintained between two consecutive minor releases. If compatibility
> cannot
> > be maintained it *MUST* be described in "Migration Guide"
> >
> > 3) Tests
> > 3.1) New functionality *MUST* be covered with unit tests for both
> positive
> > and negative use cases
> > 3.2) All test suites *MUST* be run before merge to master..There *MUST*
> be
> > no new test failures
> >
> > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
> >
> > Vladimir.
> >
> > [1] https://www.ietf.org/rfc/rfc2119.txt
> >
> > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov 
> > wrote:
> >
> > > 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
> > >> of practice. Approving of standalone refactorings instead contradicts
> > with
> > >> principle don't touch if it works. So I still like idea of co-located
> > >> changes improving code, javadocs, style, etc.
> > >>
> > >> But let's not argue about this point now, let's summarize the
> undisputed
> > >> points and add it to the wiki. Vladimir, would you please do it?
> > >>
> > >>
> > >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
> > >>
> > >> > Igniters,
> > >> >
> > >> > I agree with Vova.
> > >> >
> > >> > Don't fix if it works!
> > >> >
> > >> > If you 100% sure then it a useful addition to the product - just
> make
> > a
> > >> > separate ticket.
> > >> >
> > >> > В Ср, 25/04/2018 в 11:44 +0300, 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 org.jsr166 package in
> > >> favor
> > >> > of
> > >> > > Java 8 classes. Innocent change. Result - broken storage. Another
> > >> problem
> > >> > > is conflicts. It is not uncommon to have long-lived branches which
> > we
> > >> > need
> > >> > > to merge with master over and over again. And a lot of
> refactorings
> > >> cause
> > >> > > conflicts. It is much easier to resolve them if you know that
> logic
> > >> was
> > >> > 

Re: Ticket review checklist

[Dmitriy Setrakyan]

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 only strict rules, but *nice to have* points here as
> well with help of *MUST*, *SHOULD* and *MAY* words as per RFC2119 [1]. So
> please feel free to suggest optional items as well.
>
> 1) API
> 1.1) API compatibility *MUST* be maintained between minor releases. Do not
> remove existing methods or change their signatures, deprecate them instead
> 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> unless absolutely needed. If change is made, it *MUST* be described in
> "Migration Guide"
> 1.3) New operation *MUST* be well-documented in code (javadoc, dotnetdoc):
> documentation must contain method's purpose, description of parameters and
> how their values affect the outcome, description of return value and it's
> default, behavior in negative cases, interaction with other operations and
> components
> 1.4) API parity between Java and .NET platforms *SHOULD* be maintained when
> operation makes sense on both platforms. If method cannot be implemented in
> a platform immediately, new JIRA ticket *MUST* be created and linked to
> current ticket
> 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> when operation makes sense on several clients. If method cannot be
> implemented in a client immediately, new JIRA ticket *MUST* be created and
> linked to current ticket
> 1.6) All exceptions thrown to a user *MUST* have explanation how to
> resolve, workaround or debug an error
>
> 2) Compatibility
> 2.1) Persistence backward compatibility *MUST* be maintained between minor
> releases. It should be possible to start newer version on data files
> created by the previous version
> 2.2) Thin client forward and backward compatibility *SHOULD* be maintained
> between two consecutive minor releases. If compatibility cannot be
> maintained it *MUST* be described in "Migration Guide"
> 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> maintained between two consecutive minor releases. If compatibility cannot
> be maintained it *MUST* be described in "Migration Guide"
>
> 3) Tests
> 3.1) New functionality *MUST* be covered with unit tests for both positive
> and negative use cases
> 3.2) All test suites *MUST* be run before merge to master..There *MUST* be
> no new test failures
>
> 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
>
> Vladimir.
>
> [1] https://www.ietf.org/rfc/rfc2119.txt
>
> On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov 
> wrote:
>
> > 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
> >> of practice. Approving of standalone refactorings instead contradicts
> with
> >> principle don't touch if it works. So I still like idea of co-located
> >> changes improving code, javadocs, style, etc.
> >>
> >> But let's not argue about this point now, let's summarize the undisputed
> >> points and add it to the wiki. Vladimir, would you please do it?
> >>
> >>
> >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
> >>
> >> > Igniters,
> >> >
> >> > I agree with Vova.
> >> >
> >> > Don't fix if it works!
> >> >
> >> > If you 100% sure then it a useful addition to the product - just make
> a
> >> > separate ticket.
> >> >
> >> > В Ср, 25/04/2018 в 11:44 +0300, 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 org.jsr166 package in
> >> favor
> >> > of
> >> > > Java 8 classes. Innocent change. Result - broken storage. Another
> >> problem
> >> > > is conflicts. It is not uncommon to have long-lived branches which
> we
> >> > need
> >> > > to merge with master over and over again. And a lot of refactorings
> >> cause
> >> > > conflicts. It is much easier to resolve them if you know that logic
> >> was
> >> > not
> >> > > affected as opposed to cases when you need to resolve both renames
> and
> >> > > method extractions along with business-logic changes.
> >> > >
> >> > > I'd like to repeat - if you have a time for refactoring then you
> >> > definitely
> >> > > have a time to extract these changes to separate PR and submit a
> >> separate
> >> > > ticket. I am quite understand what "low priority" do you mean if you
> >> do
> >> > > refactorings on your own.
> >> > >
> >> > > On Tue, Apr 24, 2018 at 10:52 

Re: Ticket review checklist

[Anton Vinogradov]

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 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 RFC2119 [1]. So
> please feel free to suggest optional items as well.
>
> 1) API
> 1.1) API compatibility *MUST* be maintained between minor releases. Do not
> remove existing methods or change their signatures, deprecate them instead
> 1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
> unless absolutely needed. If change is made, it *MUST* be described in
> "Migration Guide"
> 1.3) New operation *MUST* be well-documented in code (javadoc, dotnetdoc):
> documentation must contain method's purpose, description of parameters and
> how their values affect the outcome, description of return value and it's
> default, behavior in negative cases, interaction with other operations and
> components
> 1.4) API parity between Java and .NET platforms *SHOULD* be maintained when
> operation makes sense on both platforms. If method cannot be implemented in
> a platform immediately, new JIRA ticket *MUST* be created and linked to
> current ticket
> 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
> when operation makes sense on several clients. If method cannot be
> implemented in a client immediately, new JIRA ticket *MUST* be created and
> linked to current ticket
> 1.6) All exceptions thrown to a user *MUST* have explanation how to
> resolve, workaround or debug an error
>
> 2) Compatibility
> 2.1) Persistence backward compatibility *MUST* be maintained between minor
> releases. It should be possible to start newer version on data files
> created by the previous version
> 2.2) Thin client forward and backward compatibility *SHOULD* be maintained
> between two consecutive minor releases. If compatibility cannot be
> maintained it *MUST* be described in "Migration Guide"
> 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
> maintained between two consecutive minor releases. If compatibility cannot
> be maintained it *MUST* be described in "Migration Guide"
>
> 3) Tests
> 3.1) New functionality *MUST* be covered with unit tests for both positive
> and negative use cases
> 3.2) All test suites *MUST* be run before merge to master..There *MUST* be
> no new test failures
>
> 4) Code style *MUST* be followed as per Ignite's Coding Guidelines
>
> Vladimir.
>
> [1] https://www.ietf.org/rfc/rfc2119.txt
>
> On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov 
> wrote:
>
> > 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
> >> of practice. Approving of standalone refactorings instead contradicts
> with
> >> principle don't touch if it works. So I still like idea of co-located
> >> changes improving code, javadocs, style, etc.
> >>
> >> But let's not argue about this point now, let's summarize the undisputed
> >> points and add it to the wiki. Vladimir, would you please do it?
> >>
> >>
> >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
> >>
> >> > Igniters,
> >> >
> >> > I agree with Vova.
> >> >
> >> > Don't fix if it works!
> >> >
> >> > If you 100% sure then it a useful addition to the product - just make
> a
> >> > separate ticket.
> >> >
> >> > В Ср, 25/04/2018 в 11:44 +0300, 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 org.jsr166 package in
> >> favor
> >> > of
> >> > > Java 8 classes. Innocent change. Result - broken storage. Another
> >> problem
> >> > > is conflicts. It is not uncommon to have long-lived branches which
> we
> >> > need
> >> > > to merge with master over and over again. And a lot of refactorings
> >> cause
> >> > > conflicts. It is much easier to resolve them if you know that logic
> >> was
> >> > not
> >> > > affected as opposed to cases when you need to resolve both renames
> and
> >> > > method extractions along with business-logic changes.
> >> > >
> >> > > I'd like to repeat - if you have a time for refactoring then you
> >> > definitely
> >> > > have a time to extract these changes to separate PR and submit a
> >> separate
> >> > > ticket. I am quite 

Re: Ticket review checklist

[Vladimir Ozerov]

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 RFC2119 [1]. So
please feel free to suggest optional items as well.

1) API
1.1) API compatibility *MUST* be maintained between minor releases. Do not
remove existing methods or change their signatures, deprecate them instead
1.2) Default behaviour "SHOULD NOT* be changed between minor releases,
unless absolutely needed. If change is made, it *MUST* be described in
"Migration Guide"
1.3) New operation *MUST* be well-documented in code (javadoc, dotnetdoc):
documentation must contain method's purpose, description of parameters and
how their values affect the outcome, description of return value and it's
default, behavior in negative cases, interaction with other operations and
components
1.4) API parity between Java and .NET platforms *SHOULD* be maintained when
operation makes sense on both platforms. If method cannot be implemented in
a platform immediately, new JIRA ticket *MUST* be created and linked to
current ticket
1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained
when operation makes sense on several clients. If method cannot be
implemented in a client immediately, new JIRA ticket *MUST* be created and
linked to current ticket
1.6) All exceptions thrown to a user *MUST* have explanation how to
resolve, workaround or debug an error

2) Compatibility
2.1) Persistence backward compatibility *MUST* be maintained between minor
releases. It should be possible to start newer version on data files
created by the previous version
2.2) Thin client forward and backward compatibility *SHOULD* be maintained
between two consecutive minor releases. If compatibility cannot be
maintained it *MUST* be described in "Migration Guide"
2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be
maintained between two consecutive minor releases. If compatibility cannot
be maintained it *MUST* be described in "Migration Guide"

3) Tests
3.1) New functionality *MUST* be covered with unit tests for both positive
and negative use cases
3.2) All test suites *MUST* be run before merge to master..There *MUST* be
no new test failures

4) Code style *MUST* be followed as per Ignite's Coding Guidelines

Vladimir.

[1] https://www.ietf.org/rfc/rfc2119.txt

On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov 
wrote:

> 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
>> of practice. Approving of standalone refactorings instead contradicts with
>> principle don't touch if it works. So I still like idea of co-located
>> changes improving code, javadocs, style, etc.
>>
>> But let's not argue about this point now, let's summarize the undisputed
>> points and add it to the wiki. Vladimir, would you please do it?
>>
>>
>> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
>>
>> > Igniters,
>> >
>> > I agree with Vova.
>> >
>> > Don't fix if it works!
>> >
>> > If you 100% sure then it a useful addition to the product - just make a
>> > separate ticket.
>> >
>> > В Ср, 25/04/2018 в 11:44 +0300, 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 org.jsr166 package in
>> favor
>> > of
>> > > Java 8 classes. Innocent change. Result - broken storage. Another
>> problem
>> > > is conflicts. It is not uncommon to have long-lived branches which we
>> > need
>> > > to merge with master over and over again. And a lot of refactorings
>> cause
>> > > conflicts. It is much easier to resolve them if you know that logic
>> was
>> > not
>> > > affected as opposed to cases when you need to resolve both renames and
>> > > method extractions along with business-logic changes.
>> > >
>> > > I'd like to repeat - if you have a time for refactoring then you
>> > definitely
>> > > have a time to extract these changes to separate PR and submit a
>> separate
>> > > ticket. I am quite understand what "low priority" do you mean if you
>> do
>> > > refactorings on your own.
>> > >
>> > > On Tue, Apr 24, 2018 at 10:52 PM, Andrey Kuznetsov > >
>> > > wrote:
>> > >
>> > > > +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
>> > 

Re: Ticket review checklist

[Vladimir Ozerov]

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
> of practice. Approving of standalone refactorings instead contradicts with
> principle don't touch if it works. So I still like idea of co-located
> changes improving code, javadocs, style, etc.
>
> But let's not argue about this point now, let's summarize the undisputed
> points and add it to the wiki. Vladimir, would you please do it?
>
>
> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :
>
> > Igniters,
> >
> > I agree with Vova.
> >
> > Don't fix if it works!
> >
> > If you 100% sure then it a useful addition to the product - just make a
> > separate ticket.
> >
> > В Ср, 25/04/2018 в 11:44 +0300, 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 org.jsr166 package in favor
> > of
> > > Java 8 classes. Innocent change. Result - broken storage. Another
> problem
> > > is conflicts. It is not uncommon to have long-lived branches which we
> > need
> > > to merge with master over and over again. And a lot of refactorings
> cause
> > > conflicts. It is much easier to resolve them if you know that logic was
> > not
> > > affected as opposed to cases when you need to resolve both renames and
> > > method extractions along with business-logic changes.
> > >
> > > I'd like to repeat - if you have a time for refactoring then you
> > definitely
> > > have a time to extract these changes to separate PR and submit a
> separate
> > > ticket. I am quite understand what "low priority" do you mean if you do
> > > refactorings on your own.
> > >
> > > On Tue, Apr 24, 2018 at 10:52 PM, Andrey Kuznetsov 
> > > wrote:
> > >
> > > > +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"
> pull
> > > > requests are typically rejected, since they contradict our current
> > > > guidelines.
> > > >
> > > > I understand this will require a bit more effort from
> > committer/maintainer,
> > > > but otherwise we will get constantly degrading code quality.
> > > >
> > > >
> > > > 2018-04-24 18:52 GMT+03:00 Eduard Shangareev <
> > eduard.shangar...@gmail.com
> > > > > :
> > > > > 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 6:39 PM, Vladimir Ozerov <
> > voze...@gridgain.com>
> > > > > wrote:
> > > > >
> > > > > > 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
> > him to
> > > > > > spend several minutes on JIRA and GitHub.
> > > > > >
> > > > > > As far as documentation - what you describe is normal review
> > process,
> > > > >
> > > > > when
> > > > > > reviewer might want to ask contributor to fix something.
> Checklist
> > is a
> > > > > > different thing - this is a set of rules which must be followed
> by
> > > > >
> > > > > anyone.
> > > > > > I do not understand how you can define documentation in this
> > checklist.
> > > > > > Same problem with logging - what is "enough"?
> > > > > >
> > > > > > On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev <
> > > > > > eduard.shangar...@gmail.com> wrote:
> > > > > >
> > > > > > > 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 one
> > > > > >
> > > > > > will
> > > > > > > do it.
> > > > > > >
> > > > > > >
> > > > > > > 2) Documentation.
> > > > > 

Re: Ticket review checklist

[Dmitry Pavlov]

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 co-located
changes improving code, javadocs, style, etc.

But let's not argue about this point now, let's summarize the undisputed
points and add it to the wiki. Vladimir, would you please do it?


ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov :

> Igniters,
>
> I agree with Vova.
>
> Don't fix if it works!
>
> If you 100% sure then it a useful addition to the product - just make a
> separate ticket.
>
> В Ср, 25/04/2018 в 11:44 +0300, 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 org.jsr166 package in favor
> of
> > Java 8 classes. Innocent change. Result - broken storage. Another problem
> > is conflicts. It is not uncommon to have long-lived branches which we
> need
> > to merge with master over and over again. And a lot of refactorings cause
> > conflicts. It is much easier to resolve them if you know that logic was
> not
> > affected as opposed to cases when you need to resolve both renames and
> > method extractions along with business-logic changes.
> >
> > I'd like to repeat - if you have a time for refactoring then you
> definitely
> > have a time to extract these changes to separate PR and submit a separate
> > ticket. I am quite understand what "low priority" do you mean if you do
> > refactorings on your own.
> >
> > On Tue, Apr 24, 2018 at 10:52 PM, Andrey Kuznetsov 
> > wrote:
> >
> > > +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" pull
> > > requests are typically rejected, since they contradict our current
> > > guidelines.
> > >
> > > I understand this will require a bit more effort from
> committer/maintainer,
> > > but otherwise we will get constantly degrading code quality.
> > >
> > >
> > > 2018-04-24 18:52 GMT+03:00 Eduard Shangareev <
> eduard.shangar...@gmail.com
> > > > :
> > > > 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 6:39 PM, Vladimir Ozerov <
> voze...@gridgain.com>
> > > > wrote:
> > > >
> > > > > 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
> him to
> > > > > spend several minutes on JIRA and GitHub.
> > > > >
> > > > > As far as documentation - what you describe is normal review
> process,
> > > >
> > > > when
> > > > > reviewer might want to ask contributor to fix something. Checklist
> is a
> > > > > different thing - this is a set of rules which must be followed by
> > > >
> > > > anyone.
> > > > > I do not understand how you can define documentation in this
> checklist.
> > > > > Same problem with logging - what is "enough"?
> > > > >
> > > > > On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev <
> > > > > eduard.shangar...@gmail.com> wrote:
> > > > >
> > > > > > 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 one
> > > > >
> > > > > will
> > > > > > do it.
> > > > > >
> > > > > >
> > > > > > 2) Documentation.
> > > > > > Everything which was asked by reviewers to clarify idea should be
> > > > >
> > > > > reflected
> > > > > > in the code.
> > > > > >
> > > > > > 3) Logging.
> > > > > > Logging should be enough to troubleshoot the problem if someone
> comes
> > > >
> > > > to
> > > > > > user-list with an issue in the code.
> > > > > >
> > > > > >
> > > > > > On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov <
> > >
> > > 

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 org.jsr166 package in favor of
> Java 8 classes. Innocent change. Result - broken storage. Another problem
> is conflicts. It is not uncommon to have long-lived branches which we need
> to merge with master over and over again. And a lot of refactorings cause
> conflicts. It is much easier to resolve them if you know that logic was not
> affected as opposed to cases when you need to resolve both renames and
> method extractions along with business-logic changes.
>
> I'd like to repeat - if you have a time for refactoring then you definitely
> have a time to extract these changes to separate PR and submit a separate
> ticket. I am quite understand what "low priority" do you mean if you do
> refactorings on your own.
>
> On Tue, Apr 24, 2018 at 10:52 PM, Andrey Kuznetsov 
> wrote:
>
> > +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" pull
> > requests are typically rejected, since they contradict our current
> > guidelines.
> >
> > I understand this will require a bit more effort from
> committer/maintainer,
> > but otherwise we will get constantly degrading code quality.
> >
> >
> > 2018-04-24 18:52 GMT+03:00 Eduard Shangareev <
> eduard.shangar...@gmail.com
> > >:
> >
> > > 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 6:39 PM, Vladimir Ozerov  >
> > > wrote:
> > >
> > > > 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 him
> to
> > > > spend several minutes on JIRA and GitHub.
> > > >
> > > > As far as documentation - what you describe is normal review process,
> > > when
> > > > reviewer might want to ask contributor to fix something. Checklist
> is a
> > > > different thing - this is a set of rules which must be followed by
> > > anyone.
> > > > I do not understand how you can define documentation in this
> checklist.
> > > > Same problem with logging - what is "enough"?
> > > >
> > > > On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev <
> > > > eduard.shangar...@gmail.com> wrote:
> > > >
> > > > > 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
> one
> > > > will
> > > > > do it.
> > > > >
> > > > >
> > > > > 2) Documentation.
> > > > > Everything which was asked by reviewers to clarify idea should be
> > > > reflected
> > > > > in the code.
> > > > >
> > > > > 3) Logging.
> > > > > Logging should be enough to troubleshoot the problem if someone
> comes
> > > to
> > > > > user-list with an issue in the code.
> > > > >
> > > > >
> > > > > On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov <
> > dpavlov@gmail.com>
> > > > > wrote:
> > > > >
> > > > > > 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 will have free time to submit separate patch
> > > someday
> > > > > and
> > > > > > have patience to complete patch-submission process, code will
> > remain
> > > > > > undocumented and poor-readable.
> > > > > >
> > > > > > Sincerely,
> > > > > > Dmitriy Pavlov
> > > > > >
> > > > > > пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков <
> > > sharple...@gmail.com
> > > > >:
> > > > > >
> > > > > > > 4) Metrics.
> > > > > > > partially +1
> > > > > > >
> > > > > > > It makes sense to have 

Re: Ticket review checklist

[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 org.jsr166 package in favor of
Java 8 classes. Innocent change. Result - broken storage. Another problem
is conflicts. It is not uncommon to have long-lived branches which we need
to merge with master over and over again. And a lot of refactorings cause
conflicts. It is much easier to resolve them if you know that logic was not
affected as opposed to cases when you need to resolve both renames and
method extractions along with business-logic changes.

I'd like to repeat - if you have a time for refactoring then you definitely
have a time to extract these changes to separate PR and submit a separate
ticket. I am quite understand what "low priority" do you mean if you do
refactorings on your own.

On Tue, Apr 24, 2018 at 10:52 PM, Andrey Kuznetsov 
wrote:

> +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" pull
> requests are typically rejected, since they contradict our current
> guidelines.
>
> I understand this will require a bit more effort from committer/maintainer,
> but otherwise we will get constantly degrading code quality.
>
>
> 2018-04-24 18:52 GMT+03:00 Eduard Shangareev  >:
>
> > 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 6:39 PM, Vladimir Ozerov 
> > wrote:
> >
> > > 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 him to
> > > spend several minutes on JIRA and GitHub.
> > >
> > > As far as documentation - what you describe is normal review process,
> > when
> > > reviewer might want to ask contributor to fix something. Checklist is a
> > > different thing - this is a set of rules which must be followed by
> > anyone.
> > > I do not understand how you can define documentation in this checklist.
> > > Same problem with logging - what is "enough"?
> > >
> > > On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev <
> > > eduard.shangar...@gmail.com> wrote:
> > >
> > > > 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 one
> > > will
> > > > do it.
> > > >
> > > >
> > > > 2) Documentation.
> > > > Everything which was asked by reviewers to clarify idea should be
> > > reflected
> > > > in the code.
> > > >
> > > > 3) Logging.
> > > > Logging should be enough to troubleshoot the problem if someone comes
> > to
> > > > user-list with an issue in the code.
> > > >
> > > >
> > > > On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov <
> dpavlov@gmail.com>
> > > > wrote:
> > > >
> > > > > 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 will have free time to submit separate patch
> > someday
> > > > and
> > > > > have patience to complete patch-submission process, code will
> remain
> > > > > undocumented and poor-readable.
> > > > >
> > > > > Sincerely,
> > > > > Dmitriy Pavlov
> > > > >
> > > > > пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков <
> > sharple...@gmail.com
> > > >:
> > > > >
> > > > > > 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 task.
> > > > 

Re: Ticket review checklist

[Andrey Kuznetsov]

+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" pull
requests are typically rejected, since they contradict our current
guidelines.

I understand this will require a bit more effort from committer/maintainer,
but otherwise we will get constantly degrading code quality.


2018-04-24 18:52 GMT+03:00 Eduard Shangareev :

> 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 6:39 PM, Vladimir Ozerov 
> wrote:
>
> > 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 him to
> > spend several minutes on JIRA and GitHub.
> >
> > As far as documentation - what you describe is normal review process,
> when
> > reviewer might want to ask contributor to fix something. Checklist is a
> > different thing - this is a set of rules which must be followed by
> anyone.
> > I do not understand how you can define documentation in this checklist.
> > Same problem with logging - what is "enough"?
> >
> > On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev <
> > eduard.shangar...@gmail.com> wrote:
> >
> > > 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 one
> > will
> > > do it.
> > >
> > >
> > > 2) Documentation.
> > > Everything which was asked by reviewers to clarify idea should be
> > reflected
> > > in the code.
> > >
> > > 3) Logging.
> > > Logging should be enough to troubleshoot the problem if someone comes
> to
> > > user-list with an issue in the code.
> > >
> > >
> > > On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov 
> > > wrote:
> > >
> > > > 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 will have free time to submit separate patch
> someday
> > > and
> > > > have patience to complete patch-submission process, code will remain
> > > > undocumented and poor-readable.
> > > >
> > > > Sincerely,
> > > > Dmitriy Pavlov
> > > >
> > > > пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков <
> sharple...@gmail.com
> > >:
> > > >
> > > > > 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 task.
> > > > > And it's better to remove all refactoring from PR, if it's not the
> > > sense
> > > > of
> > > > > the issue.
> > > > >
> > > > >
> > > > > 2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov :
> > > > >
> > > > > > 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
> checklist
> > > > being
> > > > > > discussed. (Of cource, refactoring should relate to problem being
> > > > > solved.)
> > > > > >
> > > > > > 2018-04-20 16:16 GMT+03:00 Vladimir Ozerov  >:
> > > > > >
> > > > > > > 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
> > > > > > >
> > > > > > > 

Re: Ticket review checklist

[Eduard Shangareev]

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 6:39 PM, Vladimir Ozerov 
wrote:

> 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 him to
> spend several minutes on JIRA and GitHub.
>
> As far as documentation - what you describe is normal review process, when
> reviewer might want to ask contributor to fix something. Checklist is a
> different thing - this is a set of rules which must be followed by anyone.
> I do not understand how you can define documentation in this checklist.
> Same problem with logging - what is "enough"?
>
> On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev <
> eduard.shangar...@gmail.com> wrote:
>
> > 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 one
> will
> > do it.
> >
> >
> > 2) Documentation.
> > Everything which was asked by reviewers to clarify idea should be
> reflected
> > in the code.
> >
> > 3) Logging.
> > Logging should be enough to troubleshoot the problem if someone comes to
> > user-list with an issue in the code.
> >
> >
> > On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov 
> > wrote:
> >
> > > 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 will have free time to submit separate patch someday
> > and
> > > have patience to complete patch-submission process, code will remain
> > > undocumented and poor-readable.
> > >
> > > Sincerely,
> > > Dmitriy Pavlov
> > >
> > > пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков  >:
> > >
> > > > 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 task.
> > > > And it's better to remove all refactoring from PR, if it's not the
> > sense
> > > of
> > > > the issue.
> > > >
> > > >
> > > > 2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov :
> > > >
> > > > > 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 checklist
> > > being
> > > > > discussed. (Of cource, refactoring should relate to problem being
> > > > solved.)
> > > > >
> > > > > 2018-04-20 16:16 GMT+03:00 Vladimir Ozerov :
> > > > >
> > > > > > 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 should
> answer
> > > the
> > > > > > question "is ticket eligible to be merged?"
> > > > > >
> > > > > > >>> 1) Code style.
> > > > > > +1
> > > > > >
> > > > > > >>>  2) Documentation
> > > > > > -1, it is impossible to define what is "well-documented". A piece
> > of
> > > > code
> > > > > > could be obvious for one contributor, and non-obvious for
> another.
> > In
> > > > any
> > > > > > case this is not a blocker for merge. Instead, during review one
> > can
> > > > ask
> > > > > > implementer to add more docs, but it cannot be forced.
> > > > > >
> > > > > > >>>  3) Logging
> > > > > > -1, same problem - what is "enough logging?". Enough for whom?
> How
> > to
> > > > > > understand whether it is enough or not?
> > > > > >
> > > > > > >>>  4) Metrics
> > > > > > -1, no clear boundaries, and decision on 

Re: Ticket review checklist

[Vladimir Ozerov]

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 him to
spend several minutes on JIRA and GitHub.

As far as documentation - what you describe is normal review process, when
reviewer might want to ask contributor to fix something. Checklist is a
different thing - this is a set of rules which must be followed by anyone.
I do not understand how you can define documentation in this checklist.
Same problem with logging - what is "enough"?

On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev <
eduard.shangar...@gmail.com> wrote:

> 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 one will
> do it.
>
>
> 2) Documentation.
> Everything which was asked by reviewers to clarify idea should be reflected
> in the code.
>
> 3) Logging.
> Logging should be enough to troubleshoot the problem if someone comes to
> user-list with an issue in the code.
>
>
> On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov 
> wrote:
>
> > 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 will have free time to submit separate patch someday
> and
> > have patience to complete patch-submission process, code will remain
> > undocumented and poor-readable.
> >
> > Sincerely,
> > Dmitriy Pavlov
> >
> > пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков :
> >
> > > 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 task.
> > > And it's better to remove all refactoring from PR, if it's not the
> sense
> > of
> > > the issue.
> > >
> > >
> > > 2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov :
> > >
> > > > 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 checklist
> > being
> > > > discussed. (Of cource, refactoring should relate to problem being
> > > solved.)
> > > >
> > > > 2018-04-20 16:16 GMT+03:00 Vladimir Ozerov :
> > > >
> > > > > 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 should answer
> > the
> > > > > question "is ticket eligible to be merged?"
> > > > >
> > > > > >>> 1) Code style.
> > > > > +1
> > > > >
> > > > > >>>  2) Documentation
> > > > > -1, it is impossible to define what is "well-documented". A piece
> of
> > > code
> > > > > could be obvious for one contributor, and non-obvious for another.
> In
> > > any
> > > > > case this is not a blocker for merge. Instead, during review one
> can
> > > ask
> > > > > implementer to add more docs, but it cannot be forced.
> > > > >
> > > > > >>>  3) Logging
> > > > > -1, same problem - what is "enough logging?". Enough for whom? How
> to
> > > > > understand whether it is enough or not?
> > > > >
> > > > > >>>  4) Metrics
> > > > > -1, no clear boundaries, and decision on whether metrics are to be
> > > added
> > > > or
> > > > > not should be performed during design phase. As before, it is
> > perfectly
> > > > > valid to ask contributor to add metrics with clear explanation why,
> > but
> > > > > this is not part of the checklist.
> > > > >
> > > > > >>> 5) TC status
> > > > > +1, already mentioned
> > > > >
> > > > > >>>  6) Refactoring
> > > > > Strong -1. OOP is a slippery slope, there are no good and bad
> > receipts
> > > > for
> > > > > all cases, hence it cannot be used in a checklist.
> > > > >
> > > > > We can borrow useful rules from p.2, p.3 and p.4 if you provide
> clear
> > > > > 

Re: Ticket review checklist

[Eduard Shangareev]

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 one will
do it.


2) Documentation.
Everything which was asked by reviewers to clarify idea should be reflected
in the code.

3) Logging.
Logging should be enough to troubleshoot the problem if someone comes to
user-list with an issue in the code.


On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov 
wrote:

> 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 will have free time to submit separate patch someday and
> have patience to complete patch-submission process, code will remain
> undocumented and poor-readable.
>
> Sincerely,
> Dmitriy Pavlov
>
> пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков :
>
> > 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 task.
> > And it's better to remove all refactoring from PR, if it's not the sense
> of
> > the issue.
> >
> >
> > 2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov :
> >
> > > 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 checklist
> being
> > > discussed. (Of cource, refactoring should relate to problem being
> > solved.)
> > >
> > > 2018-04-20 16:16 GMT+03:00 Vladimir Ozerov :
> > >
> > > > 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 should answer
> the
> > > > question "is ticket eligible to be merged?"
> > > >
> > > > >>> 1) Code style.
> > > > +1
> > > >
> > > > >>>  2) Documentation
> > > > -1, it is impossible to define what is "well-documented". A piece of
> > code
> > > > could be obvious for one contributor, and non-obvious for another. In
> > any
> > > > case this is not a blocker for merge. Instead, during review one can
> > ask
> > > > implementer to add more docs, but it cannot be forced.
> > > >
> > > > >>>  3) Logging
> > > > -1, same problem - what is "enough logging?". Enough for whom? How to
> > > > understand whether it is enough or not?
> > > >
> > > > >>>  4) Metrics
> > > > -1, no clear boundaries, and decision on whether metrics are to be
> > added
> > > or
> > > > not should be performed during design phase. As before, it is
> perfectly
> > > > valid to ask contributor to add metrics with clear explanation why,
> but
> > > > this is not part of the checklist.
> > > >
> > > > >>> 5) TC status
> > > > +1, already mentioned
> > > >
> > > > >>>  6) Refactoring
> > > > Strong -1. OOP is a slippery slope, there are no good and bad
> receipts
> > > for
> > > > all cases, hence it cannot be used in a checklist.
> > > >
> > > > We can borrow useful rules from p.2, p.3 and p.4 if you provide clear
> > > > definitions on how to measure them.
> > > >
> > > > Vladimir.
> > > >
> > > > On Fri, Apr 20, 2018 at 3:50 PM, Eduard Shangareev <
> > > > eduard.shangar...@gmail.com> wrote:
> > > >
> > > > > 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
> > > > > <
> > https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines
> > > >.
> > > > > The
> > > > > code must not contain TODOs without a ticket reference.
> > > > >
> > > > > It is highly recommended to make major formatting changes in
> existing
> > > > code
> > > > > as a separate commit, to make review process more practical.
> > > > >
> > > > > 2) Documentation.
> > > > > Added code should be well-documented. Any methods that raise
> > questions
> > > > > regarding their code flow, invariants, synchronization, etc., must
> be
> > > > > documented with comprehensive javadoc. Any reviewer can request
> that
> > a
> > > > > particular added method be 

Re: Ticket review checklist

[Dmitry Pavlov]

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 will have free time to submit separate patch someday and
have patience to complete patch-submission process, code will remain
undocumented and poor-readable.

Sincerely,
Dmitriy Pavlov

пт, 20 апр. 2018 г. в 18:56, Александр Меньшиков :

> 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 task.
> And it's better to remove all refactoring from PR, if it's not the sense of
> the issue.
>
>
> 2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov :
>
> > 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 checklist being
> > discussed. (Of cource, refactoring should relate to problem being
> solved.)
> >
> > 2018-04-20 16:16 GMT+03:00 Vladimir Ozerov :
> >
> > > 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 should answer the
> > > question "is ticket eligible to be merged?"
> > >
> > > >>> 1) Code style.
> > > +1
> > >
> > > >>>  2) Documentation
> > > -1, it is impossible to define what is "well-documented". A piece of
> code
> > > could be obvious for one contributor, and non-obvious for another. In
> any
> > > case this is not a blocker for merge. Instead, during review one can
> ask
> > > implementer to add more docs, but it cannot be forced.
> > >
> > > >>>  3) Logging
> > > -1, same problem - what is "enough logging?". Enough for whom? How to
> > > understand whether it is enough or not?
> > >
> > > >>>  4) Metrics
> > > -1, no clear boundaries, and decision on whether metrics are to be
> added
> > or
> > > not should be performed during design phase. As before, it is perfectly
> > > valid to ask contributor to add metrics with clear explanation why, but
> > > this is not part of the checklist.
> > >
> > > >>> 5) TC status
> > > +1, already mentioned
> > >
> > > >>>  6) Refactoring
> > > Strong -1. OOP is a slippery slope, there are no good and bad receipts
> > for
> > > all cases, hence it cannot be used in a checklist.
> > >
> > > We can borrow useful rules from p.2, p.3 and p.4 if you provide clear
> > > definitions on how to measure them.
> > >
> > > Vladimir.
> > >
> > > On Fri, Apr 20, 2018 at 3:50 PM, Eduard Shangareev <
> > > eduard.shangar...@gmail.com> wrote:
> > >
> > > > 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
> > > > <
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines
> > >.
> > > > The
> > > > code must not contain TODOs without a ticket reference.
> > > >
> > > > It is highly recommended to make major formatting changes in existing
> > > code
> > > > as a separate commit, to make review process more practical.
> > > >
> > > > 2) Documentation.
> > > > Added code should be well-documented. Any methods that raise
> questions
> > > > regarding their code flow, invariants, synchronization, etc., must be
> > > > documented with comprehensive javadoc. Any reviewer can request that
> a
> > > > particular added method be documented. Also, it is a good practice to
> > > > document old code in a 10-20 lines region around changed code.
> > > >
> > > > 3) Logging.
> > > > Make sure that there are enough logging added in every category for
> > > > possible diagnostic in field. Check that logging messages are
> properly
> > > > spelled.
> > > >
> > > > 4) Metrics.
> > > > Are there any metrics that need to be exposed to user?
> > > >
> > > > 5) TC status.
> > > >
> > > > Recheck that there are no new failing tests
> > > >
> > > > 6) Refactoring.
> > > >
> > > > The code should be better than before:
> > > >
> > > >- extract method from big one;
> > > >- do anything else to make code clearer (don't forget about some
> > > >OOP-practise, replace if-else hell with inheritance
> > > >- split refactoring (renaming, code format) from actual changes by
> > > >separate commit
> > > >

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 task.
And it's better to remove all refactoring from PR, if it's not the sense of
the issue.


2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov :

> 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 checklist being
> discussed. (Of cource, refactoring should relate to problem being solved.)
>
> 2018-04-20 16:16 GMT+03:00 Vladimir Ozerov :
>
> > 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 should answer the
> > question "is ticket eligible to be merged?"
> >
> > >>> 1) Code style.
> > +1
> >
> > >>>  2) Documentation
> > -1, it is impossible to define what is "well-documented". A piece of code
> > could be obvious for one contributor, and non-obvious for another. In any
> > case this is not a blocker for merge. Instead, during review one can ask
> > implementer to add more docs, but it cannot be forced.
> >
> > >>>  3) Logging
> > -1, same problem - what is "enough logging?". Enough for whom? How to
> > understand whether it is enough or not?
> >
> > >>>  4) Metrics
> > -1, no clear boundaries, and decision on whether metrics are to be added
> or
> > not should be performed during design phase. As before, it is perfectly
> > valid to ask contributor to add metrics with clear explanation why, but
> > this is not part of the checklist.
> >
> > >>> 5) TC status
> > +1, already mentioned
> >
> > >>>  6) Refactoring
> > Strong -1. OOP is a slippery slope, there are no good and bad receipts
> for
> > all cases, hence it cannot be used in a checklist.
> >
> > We can borrow useful rules from p.2, p.3 and p.4 if you provide clear
> > definitions on how to measure them.
> >
> > Vladimir.
> >
> > On Fri, Apr 20, 2018 at 3:50 PM, Eduard Shangareev <
> > eduard.shangar...@gmail.com> wrote:
> >
> > > 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 recommended to make major formatting changes in existing
> > code
> > > as a separate commit, to make review process more practical.
> > >
> > > 2) Documentation.
> > > Added code should be well-documented. Any methods that raise questions
> > > regarding their code flow, invariants, synchronization, etc., must be
> > > documented with comprehensive javadoc. Any reviewer can request that a
> > > particular added method be documented. Also, it is a good practice to
> > > document old code in a 10-20 lines region around changed code.
> > >
> > > 3) Logging.
> > > Make sure that there are enough logging added in every category for
> > > possible diagnostic in field. Check that logging messages are properly
> > > spelled.
> > >
> > > 4) Metrics.
> > > Are there any metrics that need to be exposed to user?
> > >
> > > 5) TC status.
> > >
> > > Recheck that there are no new failing tests
> > >
> > > 6) Refactoring.
> > >
> > > The code should be better than before:
> > >
> > >- extract method from big one;
> > >- do anything else to make code clearer (don't forget about some
> > >OOP-practise, replace if-else hell with inheritance
> > >- split refactoring (renaming, code format) from actual changes by
> > >separate commit
> > >
> > >
> > >
> > >
> > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > eduard.shangar...@gmail.com> wrote:
> > >
> > > > 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, Anton Vinogradov 
> > wrote:
> > > >
> > > >> 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 

Re: Ticket review checklist

[Andrey Kuznetsov]

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 checklist being
discussed. (Of cource, refactoring should relate to problem being solved.)

2018-04-20 16:16 GMT+03:00 Vladimir Ozerov :

> 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 should answer the
> question "is ticket eligible to be merged?"
>
> >>> 1) Code style.
> +1
>
> >>>  2) Documentation
> -1, it is impossible to define what is "well-documented". A piece of code
> could be obvious for one contributor, and non-obvious for another. In any
> case this is not a blocker for merge. Instead, during review one can ask
> implementer to add more docs, but it cannot be forced.
>
> >>>  3) Logging
> -1, same problem - what is "enough logging?". Enough for whom? How to
> understand whether it is enough or not?
>
> >>>  4) Metrics
> -1, no clear boundaries, and decision on whether metrics are to be added or
> not should be performed during design phase. As before, it is perfectly
> valid to ask contributor to add metrics with clear explanation why, but
> this is not part of the checklist.
>
> >>> 5) TC status
> +1, already mentioned
>
> >>>  6) Refactoring
> Strong -1. OOP is a slippery slope, there are no good and bad receipts for
> all cases, hence it cannot be used in a checklist.
>
> We can borrow useful rules from p.2, p.3 and p.4 if you provide clear
> definitions on how to measure them.
>
> Vladimir.
>
> On Fri, Apr 20, 2018 at 3:50 PM, Eduard Shangareev <
> eduard.shangar...@gmail.com> wrote:
>
> > 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 recommended to make major formatting changes in existing
> code
> > as a separate commit, to make review process more practical.
> >
> > 2) Documentation.
> > Added code should be well-documented. Any methods that raise questions
> > regarding their code flow, invariants, synchronization, etc., must be
> > documented with comprehensive javadoc. Any reviewer can request that a
> > particular added method be documented. Also, it is a good practice to
> > document old code in a 10-20 lines region around changed code.
> >
> > 3) Logging.
> > Make sure that there are enough logging added in every category for
> > possible diagnostic in field. Check that logging messages are properly
> > spelled.
> >
> > 4) Metrics.
> > Are there any metrics that need to be exposed to user?
> >
> > 5) TC status.
> >
> > Recheck that there are no new failing tests
> >
> > 6) Refactoring.
> >
> > The code should be better than before:
> >
> >- extract method from big one;
> >- do anything else to make code clearer (don't forget about some
> >OOP-practise, replace if-else hell with inheritance
> >- split refactoring (renaming, code format) from actual changes by
> >separate commit
> >
> >
> >
> >
> > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > eduard.shangar...@gmail.com> wrote:
> >
> > > 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, Anton Vinogradov 
> wrote:
> > >
> > >> 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 :
> > >>
> > >> > 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 should approve the changes
> > >> >
> > >> > We can learn from other open source project experience.
> > >> > Apache Kafka [1], for example, requires KIP(kafka improvement
> 

Re: Ticket review checklist

[Vladimir Ozerov]

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 should answer the
question "is ticket eligible to be merged?"

>>> 1) Code style.
+1

>>>  2) Documentation
-1, it is impossible to define what is "well-documented". A piece of code
could be obvious for one contributor, and non-obvious for another. In any
case this is not a blocker for merge. Instead, during review one can ask
implementer to add more docs, but it cannot be forced.

>>>  3) Logging
-1, same problem - what is "enough logging?". Enough for whom? How to
understand whether it is enough or not?

>>>  4) Metrics
-1, no clear boundaries, and decision on whether metrics are to be added or
not should be performed during design phase. As before, it is perfectly
valid to ask contributor to add metrics with clear explanation why, but
this is not part of the checklist.

>>> 5) TC status
+1, already mentioned

>>>  6) Refactoring
Strong -1. OOP is a slippery slope, there are no good and bad receipts for
all cases, hence it cannot be used in a checklist.

We can borrow useful rules from p.2, p.3 and p.4 if you provide clear
definitions on how to measure them.

Vladimir.

On Fri, Apr 20, 2018 at 3:50 PM, Eduard Shangareev <
eduard.shangar...@gmail.com> wrote:

> 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 recommended to make major formatting changes in existing code
> as a separate commit, to make review process more practical.
>
> 2) Documentation.
> Added code should be well-documented. Any methods that raise questions
> regarding their code flow, invariants, synchronization, etc., must be
> documented with comprehensive javadoc. Any reviewer can request that a
> particular added method be documented. Also, it is a good practice to
> document old code in a 10-20 lines region around changed code.
>
> 3) Logging.
> Make sure that there are enough logging added in every category for
> possible diagnostic in field. Check that logging messages are properly
> spelled.
>
> 4) Metrics.
> Are there any metrics that need to be exposed to user?
>
> 5) TC status.
>
> Recheck that there are no new failing tests
>
> 6) Refactoring.
>
> The code should be better than before:
>
>- extract method from big one;
>- do anything else to make code clearer (don't forget about some
>OOP-practise, replace if-else hell with inheritance
>- split refactoring (renaming, code format) from actual changes by
>separate commit
>
>
>
>
> On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> eduard.shangar...@gmail.com> wrote:
>
> > 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, Anton Vinogradov  wrote:
> >
> >> 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 :
> >>
> >> > 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 should approve the changes
> >> >
> >> > We can learn from other open source project experience.
> >> > Apache Kafka [1], for example, requires KIP(kafka improvement
> proposal)
> >> > for *every* major change.
> >> > Major change definition includes public API.
> >> >
> >> > [1] https://cwiki.apache.org/confluence/display/KAFKA/
> >> > Kafka+Improvement+Proposals
> >> >
> >> >
> >> > В Чт, 19/04/2018 в 23:00 +0300, Vladimir Ozerov пишет:
> >> > > Hi Igniters,
> >> > >
> >> > > It's glad to see our community becomes larger every day. But as it
> >> grows
> >> > it
> >> > > becomes more and more difficult to manage review and merge processes
> >> and
> >> > > keep quality of our decisions at the proper level. More
> contributors,
> >> > more
> >> > > commits, more components interlinked with each other in subtle ways.
> >> > >
> >> > > I would like to propose to setup 

Re: Ticket review checklist

[Eduard Shangareev]

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 recommended to make major formatting changes in existing code
as a separate commit, to make review process more practical.

2) Documentation.
Added code should be well-documented. Any methods that raise questions
regarding their code flow, invariants, synchronization, etc., must be
documented with comprehensive javadoc. Any reviewer can request that a
particular added method be documented. Also, it is a good practice to
document old code in a 10-20 lines region around changed code.

3) Logging.
Make sure that there are enough logging added in every category for
possible diagnostic in field. Check that logging messages are properly
spelled.

4) Metrics.
Are there any metrics that need to be exposed to user?

5) TC status.

Recheck that there are no new failing tests

6) Refactoring.

The code should be better than before:

   - extract method from big one;
   - do anything else to make code clearer (don't forget about some
   OOP-practise, replace if-else hell with inheritance
   - split refactoring (renaming, code format) from actual changes by
   separate commit




On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
eduard.shangar...@gmail.com> wrote:

> 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, Anton Vinogradov  wrote:
>
>> 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 :
>>
>> > 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 should approve the changes
>> >
>> > We can learn from other open source project experience.
>> > Apache Kafka [1], for example, requires KIP(kafka improvement proposal)
>> > for *every* major change.
>> > Major change definition includes public API.
>> >
>> > [1] https://cwiki.apache.org/confluence/display/KAFKA/
>> > Kafka+Improvement+Proposals
>> >
>> >
>> > В Чт, 19/04/2018 в 23:00 +0300, Vladimir Ozerov пишет:
>> > > Hi Igniters,
>> > >
>> > > It's glad to see our community becomes larger every day. But as it
>> grows
>> > it
>> > > becomes more and more difficult to manage review and merge processes
>> and
>> > > keep quality of our decisions at the proper level. More contributors,
>> > more
>> > > commits, more components interlinked with each other in subtle ways.
>> > >
>> > > I would like to propose to setup a formal review checklist. This would
>> > be a
>> > > set of actions every reviewer needs to check before approving merge
>> of a
>> > > certain feature. Passing the checklist would be *necessary but not
>> > > sufficient* phase before commit could be added to the main branch. The
>> > > checklist would help us to detect a lot of common problems such a
>> broken
>> > > tests or bad UX earlier, and would help contributors lead their pull
>> > > requests to merge.
>> > >
>> > > Hallmarks of a good checklist:
>> > > - It must be followed be everyone without exceptions
>> > > - It must be clear and disallow multiple interpretations
>> > > - It must be lightweight, otherwise Ignite development would become a
>> > > nightmare
>> > > - It must be non-blocking, i.e. inacessibility of a single contributor
>> > > should not block ticket progress forever.
>> > >
>> > > Please let me know if you think the idea makes sense. If we agree on
>> it,
>> > > let's start defining action items for the checklist. My 2 cents:
>> > > 1) All unit tests pass on TC without new failures
>> > > 2) If ticket targets specific component, it should be reviewed by
>> > > component's maintainer*
>> > > 3) If ticket changes public API or existing behavior, at least two
>> > > commiters should approve the changes **
>> > >
>> > > Thoughts?
>> > >
>> > > Vladimir.
>> > >
>> > > * TBD: Review component list and define maintainers; define what to
>> do if
>> > > maintainer is unavailable
>> > > ** TBD: Define what is "public API"
>> >
>>
>
>

Re: Ticket review checklist

[Eduard Shangareev]

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, Anton Vinogradov  wrote:

> 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 :
>
> > 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 should approve the changes
> >
> > We can learn from other open source project experience.
> > Apache Kafka [1], for example, requires KIP(kafka improvement proposal)
> > for *every* major change.
> > Major change definition includes public API.
> >
> > [1] https://cwiki.apache.org/confluence/display/KAFKA/
> > Kafka+Improvement+Proposals
> >
> >
> > В Чт, 19/04/2018 в 23:00 +0300, Vladimir Ozerov пишет:
> > > Hi Igniters,
> > >
> > > It's glad to see our community becomes larger every day. But as it
> grows
> > it
> > > becomes more and more difficult to manage review and merge processes
> and
> > > keep quality of our decisions at the proper level. More contributors,
> > more
> > > commits, more components interlinked with each other in subtle ways.
> > >
> > > I would like to propose to setup a formal review checklist. This would
> > be a
> > > set of actions every reviewer needs to check before approving merge of
> a
> > > certain feature. Passing the checklist would be *necessary but not
> > > sufficient* phase before commit could be added to the main branch. The
> > > checklist would help us to detect a lot of common problems such a
> broken
> > > tests or bad UX earlier, and would help contributors lead their pull
> > > requests to merge.
> > >
> > > Hallmarks of a good checklist:
> > > - It must be followed be everyone without exceptions
> > > - It must be clear and disallow multiple interpretations
> > > - It must be lightweight, otherwise Ignite development would become a
> > > nightmare
> > > - It must be non-blocking, i.e. inacessibility of a single contributor
> > > should not block ticket progress forever.
> > >
> > > Please let me know if you think the idea makes sense. If we agree on
> it,
> > > let's start defining action items for the checklist. My 2 cents:
> > > 1) All unit tests pass on TC without new failures
> > > 2) If ticket targets specific component, it should be reviewed by
> > > component's maintainer*
> > > 3) If ticket changes public API or existing behavior, at least two
> > > commiters should approve the changes **
> > >
> > > Thoughts?
> > >
> > > Vladimir.
> > >
> > > * TBD: Review component list and define maintainers; define what to do
> if
> > > maintainer is unavailable
> > > ** TBD: Define what is "public API"
> >
>

Re: Ticket review checklist

[Anton Vinogradov]

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 :

> 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 should approve the changes
>
> We can learn from other open source project experience.
> Apache Kafka [1], for example, requires KIP(kafka improvement proposal)
> for *every* major change.
> Major change definition includes public API.
>
> [1] https://cwiki.apache.org/confluence/display/KAFKA/
> Kafka+Improvement+Proposals
>
>
> В Чт, 19/04/2018 в 23:00 +0300, Vladimir Ozerov пишет:
> > Hi Igniters,
> >
> > It's glad to see our community becomes larger every day. But as it grows
> it
> > becomes more and more difficult to manage review and merge processes and
> > keep quality of our decisions at the proper level. More contributors,
> more
> > commits, more components interlinked with each other in subtle ways.
> >
> > I would like to propose to setup a formal review checklist. This would
> be a
> > set of actions every reviewer needs to check before approving merge of a
> > certain feature. Passing the checklist would be *necessary but not
> > sufficient* phase before commit could be added to the main branch. The
> > checklist would help us to detect a lot of common problems such a broken
> > tests or bad UX earlier, and would help contributors lead their pull
> > requests to merge.
> >
> > Hallmarks of a good checklist:
> > - It must be followed be everyone without exceptions
> > - It must be clear and disallow multiple interpretations
> > - It must be lightweight, otherwise Ignite development would become a
> > nightmare
> > - It must be non-blocking, i.e. inacessibility of a single contributor
> > should not block ticket progress forever.
> >
> > Please let me know if you think the idea makes sense. If we agree on it,
> > let's start defining action items for the checklist. My 2 cents:
> > 1) All unit tests pass on TC without new failures
> > 2) If ticket targets specific component, it should be reviewed by
> > component's maintainer*
> > 3) If ticket changes public API or existing behavior, at least two
> > commiters should approve the changes **
> >
> > Thoughts?
> >
> > Vladimir.
> >
> > * TBD: Review component list and define maintainers; define what to do if
> > maintainer is unavailable
> > ** TBD: Define what is "public API"
>

Re: Ticket review checklist

[Nikolay Izhikov]

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 
> should approve the changes

We can learn from other open source project experience.
Apache Kafka [1], for example, requires KIP(kafka improvement proposal) for 
*every* major change.
Major change definition includes public API.

[1] 
https://cwiki.apache.org/confluence/display/KAFKA/Kafka+Improvement+Proposals


В Чт, 19/04/2018 в 23:00 +0300, Vladimir Ozerov пишет:
> Hi Igniters,
> 
> It's glad to see our community becomes larger every day. But as it grows it
> becomes more and more difficult to manage review and merge processes and
> keep quality of our decisions at the proper level. More contributors, more
> commits, more components interlinked with each other in subtle ways.
> 
> I would like to propose to setup a formal review checklist. This would be a
> set of actions every reviewer needs to check before approving merge of a
> certain feature. Passing the checklist would be *necessary but not
> sufficient* phase before commit could be added to the main branch. The
> checklist would help us to detect a lot of common problems such a broken
> tests or bad UX earlier, and would help contributors lead their pull
> requests to merge.
> 
> Hallmarks of a good checklist:
> - It must be followed be everyone without exceptions
> - It must be clear and disallow multiple interpretations
> - It must be lightweight, otherwise Ignite development would become a
> nightmare
> - It must be non-blocking, i.e. inacessibility of a single contributor
> should not block ticket progress forever.
> 
> Please let me know if you think the idea makes sense. If we agree on it,
> let's start defining action items for the checklist. My 2 cents:
> 1) All unit tests pass on TC without new failures
> 2) If ticket targets specific component, it should be reviewed by
> component's maintainer*
> 3) If ticket changes public API or existing behavior, at least two
> commiters should approve the changes **
> 
> Thoughts?
> 
> Vladimir.
> 
> * TBD: Review component list and define maintainers; define what to do if
> maintainer is unavailable
> ** TBD: Define what is "public API"

signature.asc
Description: This is a digitally signed message part

Ticket review checklist

[Vladimir Ozerov]

Hi Igniters,

It's glad to see our community becomes larger every day. But as it grows it
becomes more and more difficult to manage review and merge processes and
keep quality of our decisions at the proper level. More contributors, more
commits, more components interlinked with each other in subtle ways.

I would like to propose to setup a formal review checklist. This would be a
set of actions every reviewer needs to check before approving merge of a
certain feature. Passing the checklist would be *necessary but not
sufficient* phase before commit could be added to the main branch. The
checklist would help us to detect a lot of common problems such a broken
tests or bad UX earlier, and would help contributors lead their pull
requests to merge.

Hallmarks of a good checklist:
- It must be followed be everyone without exceptions
- It must be clear and disallow multiple interpretations
- It must be lightweight, otherwise Ignite development would become a
nightmare
- It must be non-blocking, i.e. inacessibility of a single contributor
should not block ticket progress forever.

Please let me know if you think the idea makes sense. If we agree on it,
let's start defining action items for the checklist. My 2 cents:
1) All unit tests pass on TC without new failures
2) If ticket targets specific component, it should be reviewed by
component's maintainer*
3) If ticket changes public API or existing behavior, at least two
commiters should approve the changes **

Thoughts?

Vladimir.

* TBD: Review component list and define maintainers; define what to do if
maintainer is unavailable
** TBD: Define what is "public API"