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"

[Nikolay Izhikov-2]

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"

[Anton Vinogradov-2]

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 <[hidden email]>:

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

[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 <[hidden email]> 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 <[hidden email]>:
>
> > 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"
> >
>

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




On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
[hidden email]> 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 <[hidden email]> 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 <[hidden email]>:
>>
>> > 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"
>> >
>>
>
>

[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 <
[hidden email]> 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
>
>
>
>
> On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> [hidden email]> 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 <[hidden email]> 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 <[hidden email]>:
> >>
> >> > 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"
> >> >
> >>
> >
> >
>

[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 <[hidden email]>:

> 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 <
> [hidden email]> 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
> >
> >
> >
> >
> > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > [hidden email]> 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 <[hidden email]>
> 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 <[hidden email]>:
> > >>
> > >> > 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"
> > >> >
> > >>
> > >
> > >
> >
>

--
Best regards,
  Andrey Kuznetsov.

[Александр Меньшиков]

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 <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <
> > [hidden email]> 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
> > >
> > >
> > >
> > >
> > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > [hidden email]> 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 <[hidden email]>
> > 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 <[hidden email]>:
> > > >>
> > > >> > 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"
> > > >> >
> > > >>
> > > >
> > > >
> > >
> >
>
>
>
> --
> Best regards,
>   Andrey Kuznetsov.
>

[Dmitriy 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, Александр Меньшиков <[hidden email]>:

> 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 <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > 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 <
> > > [hidden email]> 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
> > > >
> > > >
> > > >
> > > >
> > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > [hidden email]> 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 <[hidden email]>
> > > 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 <[hidden email]>:
> > > > >>
> > > > >> > 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"
> > > > >> >
> > > > >>
> > > > >
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> > Best regards,
> >   Andrey Kuznetsov.
> >
>

[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 <[hidden email]>
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, Александр Меньшиков <[hidden email]>:
>
> > 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 <[hidden email]>:
> >
> > > 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 <[hidden email]>:
> > >
> > > > 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 <
> > > > [hidden email]> 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
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > [hidden email]> 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 <[hidden email]
> >
> > > > 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 <[hidden email]
> >:
> > > > > >>
> > > > > >> > 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"
> > > > > >> >
> > > > > >>
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > >   Andrey Kuznetsov.
> > >
> >
>

[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 <
[hidden email]> 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 <[hidden email]>
> 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, Александр Меньшиков <[hidden email]>:
> >
> > > 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 <[hidden email]>:
> > >
> > > > 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 <[hidden email]>:
> > > >
> > > > > 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 <
> > > > > [hidden email]> 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
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > [hidden email]> 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 <
> [hidden email]
> > >
> > > > > 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 <
> [hidden email]
> > >:
> > > > > > >>
> > > > > > >> > 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"
> > > > > > >> >
> > > > > > >>
> > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > >   Andrey Kuznetsov.
> > > >
> > >
> >
>

[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 <[hidden email]>
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 <
> [hidden email]> 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 <[hidden email]>
> > 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, Александр Меньшиков <[hidden email]
> >:
> > >
> > > > 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 <[hidden email]>:
> > > >
> > > > > 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 <[hidden email]>:
> > > > >
> > > > > > 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 <
> > > > > > [hidden email]> 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
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > > [hidden email]> 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 <
> > [hidden email]
> > > >
> > > > > > 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 <
> > [hidden email]
> > > >:
> > > > > > > >>
> > > > > > > >> > 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"
> > > > > > > >> >
> > > > > > > >>
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best regards,
> > > > >   Andrey Kuznetsov.
> > > > >
> > > >
> > >
> >
>

[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 <[hidden email]>:

> 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 <[hidden email]>
> 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 <
> > [hidden email]> 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 <[hidden email]>
> > > 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, Александр Меньшиков <
> [hidden email]
> > >:
> > > >
> > > > > 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 <[hidden email]>:
> > > > >
> > > > > > 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 <[hidden email]
> >:
> > > > > >
> > > > > > > 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 <
> > > > > > > [hidden email]> 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
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > > > [hidden email]> 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 <
> > > [hidden email]
> > > > >
> > > > > > > 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 <
> > > [hidden email]
> > > > >:
> > > > > > > > >>
> > > > > > > > >> > 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"
> > > > > > > > >> >
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Best regards,
> > > > > >   Andrey Kuznetsov.
> > > > > >
> > > > >
> > > >
> > >
> >
>

--
Best regards,
  Andrey Kuznetsov.

[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 <[hidden email]>
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 <[hidden email]
> >:
>
> > 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 <[hidden email]>
> > 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 <
> > > [hidden email]> 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 <
> [hidden email]>
> > > > 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, Александр Меньшиков <
> > [hidden email]
> > > >:
> > > > >
> > > > > > 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 <[hidden email]>:
> > > > > >
> > > > > > > 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 <
> [hidden email]
> > >:
> > > > > > >
> > > > > > > > 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 <
> > > > > > > > [hidden email]> 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
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > > > > [hidden email]> 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 <
> > > > [hidden email]
> > > > > >
> > > > > > > > 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 <
> > > > [hidden email]
> > > > > >:
> > > > > > > > > >>
> > > > > > > > > >> > 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"
> > > > > > > > > >> >
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Best regards,
> > > > > > >   Andrey Kuznetsov.
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
>
>
> --
> Best regards,
>   Andrey Kuznetsov.
>

[Александр Меньшиков]

+1 to Vladimir Ozerov

2018-04-25 11:44 GMT+03:00 Vladimir Ozerov <[hidden email]>:

> 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 <[hidden email]>
> 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 <
> [hidden email]
> > >:
> >
> > > 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 <[hidden email]
> >
> > > 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 <
> > > > [hidden email]> 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 <
> > [hidden email]>
> > > > > 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, Александр Меньшиков <
> > > [hidden email]
> > > > >:
> > > > > >
> > > > > > > 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 <[hidden email]
> >:
> > > > > > >
> > > > > > > > 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 <
> > [hidden email]
> > > >:
> > > > > > > >
> > > > > > > > > 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 <
> > > > > > > > > [hidden email]> 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
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > > > > > [hidden email]> 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 <
> > > > > [hidden email]
> > > > > > >
> > > > > > > > > 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 <
> > > > > [hidden email]
> > > > > > >:
> > > > > > > > > > >>
> > > > > > > > > > >> > 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"
> > > > > > > > > > >> >
> > > > > > > > > > >>
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Best regards,
> > > > > > > >   Andrey Kuznetsov.
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
> >
> >
> > --
> > Best regards,
> >   Andrey Kuznetsov.
> >
>

[Nikolay Izhikov-2]

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 <[hidden email]>
> 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 <[hidden email]
> > > :
> > > 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 <[hidden email]>
> > > 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 <
> > > > [hidden email]> 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 <
> >
> > [hidden email]>
> > > > > 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, Александр Меньшиков <
> > >
> > > [hidden email]
> > > > > :
> > > > > >
> > > > > > > 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 <[hidden email]>:
> > > > > > >
> > > > > > > > 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 <
> >
> > [hidden email]
> > > > :
> > > > > > > >
> > > > > > > > > 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 <
> > > > > > > > > [hidden email]> 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
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > > > > > [hidden email]> 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 <
> > > > >
> > > > > [hidden email]
> > > > > > >
> > > > > > > > > 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 <
> > > > >
> > > > > [hidden email]
> > > > > > > :
> > > > > > > > > > > >
> > > > > > > > > > > > > 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"
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Best regards,
> > > > > > > >   Andrey Kuznetsov.
> > > > > > > >
> >
> >
> >
> > --
> > Best regards,
> >   Andrey Kuznetsov.
> >

[Dmitriy 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 <[hidden email]>:

> 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 <[hidden email]>
> > 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 <
> [hidden email]
> > > > :
> > > > 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 <
> [hidden email]>
> > > > 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 <
> > > > > [hidden email]> 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 <
> > >
> > > [hidden email]>
> > > > > > 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, Александр Меньшиков <
> > > >
> > > > [hidden email]
> > > > > > :
> > > > > > >
> > > > > > > > 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 <
> [hidden email]>:
> > > > > > > >
> > > > > > > > > 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 <
> > >
> > > [hidden email]
> > > > > :
> > > > > > > > >
> > > > > > > > > > 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 <
> > > > > > > > > > [hidden email]> 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
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > > > > > > [hidden email]> 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 <
> > > > > >
> > > > > > [hidden email]
> > > > > > > >
> > > > > > > > > > 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 <
> > > > > >
> > > > > > [hidden email]
> > > > > > > > :
> > > > > > > > > > > > >
> > > > > > > > > > > > > > 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"
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Best regards,
> > > > > > > > >   Andrey Kuznetsov.
> > > > > > > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > >   Andrey Kuznetsov.
> > >

[Vladimir Ozerov]

Hi Dmitry,

Yes, I'll do that in the nearest days.

On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <[hidden email]>
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 <[hidden email]>:
>
> > 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 <[hidden email]>
> > > 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 <
> > [hidden email]
> > > > > :
> > > > > 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 <
> > [hidden email]>
> > > > > 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 <
> > > > > > [hidden email]> 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 <
> > > >
> > > > [hidden email]>
> > > > > > > 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, Александр Меньшиков <
> > > > >
> > > > > [hidden email]
> > > > > > > :
> > > > > > > >
> > > > > > > > > 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 <
> > [hidden email]>:
> > > > > > > > >
> > > > > > > > > > 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 <
> > > >
> > > > [hidden email]
> > > > > > :
> > > > > > > > > >
> > > > > > > > > > > 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 <
> > > > > > > > > > > [hidden email]> 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
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > > > > > > > > > [hidden email]> 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 <
> > > > > > >
> > > > > > > [hidden email]
> > > > > > > > >
> > > > > > > > > > > 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 <
> > > > > > >
> > > > > > > [hidden email]
> > > > > > > > > :
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > > 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"
> > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > --
> > > > > > > > > > Best regards,
> > > > > > > > > >   Andrey Kuznetsov.
> > > > > > > > > >
> > > >
> > > >
> > > >
> > > > --
> > > > Best regards,
> > > >   Andrey Kuznetsov.
> > > >
>

[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 <[hidden email]>
wrote:

> Hi Dmitry,
>
> Yes, I'll do that in the nearest days.
>
> On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <[hidden email]>
> 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 <[hidden email]>:
>>
>> > 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 <[hidden email]
>> >
>> > > 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 <
>> > [hidden email]
>> > > > > :
>> > > > > 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 <
>> > [hidden email]>
>> > > > > 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 <
>> > > > > > [hidden email]> 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 <
>> > > >
>> > > > [hidden email]>
>> > > > > > > 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, Александр Меньшиков <
>> > > > >
>> > > > > [hidden email]
>> > > > > > > :
>> > > > > > > >
>> > > > > > > > > 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 <
>> > [hidden email]>:
>> > > > > > > > >
>> > > > > > > > > > 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 <
>> > > >
>> > > > [hidden email]
>> > > > > > :
>> > > > > > > > > >
>> > > > > > > > > > > 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 <
>> > > > > > > > > > > [hidden email]> 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
>> > > > > > > > > > > >
>> > > > > > > > > > > >
>> > > > > > > > > > > >
>> > > > > > > > > > > >
>> > > > > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
>> > > > > > > > > > > > [hidden email]> 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
>> <
>> > > > > > >
>> > > > > > > [hidden email]
>> > > > > > > > >
>> > > > > > > > > > > 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 <
>> > > > > > >
>> > > > > > > [hidden email]
>> > > > > > > > > :
>> > > > > > > > > > > > > >
>> > > > > > > > > > > > > > > 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"
>> > > > > > > > > > > > >
>> > > > > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > >
>> > > > > > > > > > --
>> > > > > > > > > > Best regards,
>> > > > > > > > > >   Andrey Kuznetsov.
>> > > > > > > > > >
>> > > >
>> > > >
>> > > >
>> > > > --
>> > > > Best regards,
>> > > >   Andrey Kuznetsov.
>> > > >
>>
>
>

[Anton Vinogradov-2]

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 <[hidden email]>:

> 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 <[hidden email]>
> wrote:
>
> > Hi Dmitry,
> >
> > Yes, I'll do that in the nearest days.
> >
> > On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <[hidden email]>
> > 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 <[hidden email]>:
> >>
> >> > 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 <
> [hidden email]
> >> >
> >> > > 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 <
> >> > [hidden email]
> >> > > > > :
> >> > > > > 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 <
> >> > [hidden email]>
> >> > > > > 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 <
> >> > > > > > [hidden email]> 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 <
> >> > > >
> >> > > > [hidden email]>
> >> > > > > > > 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, Александр Меньшиков <
> >> > > > >
> >> > > > > [hidden email]
> >> > > > > > > :
> >> > > > > > > >
> >> > > > > > > > > 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 <
> >> > [hidden email]>:
> >> > > > > > > > >
> >> > > > > > > > > > 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 <
> >> > > >
> >> > > > [hidden email]
> >> > > > > > :
> >> > > > > > > > > >
> >> > > > > > > > > > > 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 <
> >> > > > > > > > > > > [hidden email]> 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
> >> > > > > > > > > > > >
> >> > > > > > > > > > > >
> >> > > > > > > > > > > >
> >> > > > > > > > > > > >
> >> > > > > > > > > > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard
> Shangareev <
> >> > > > > > > > > > > > [hidden email]> 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
> >> <
> >> > > > > > >
> >> > > > > > > [hidden email]
> >> > > > > > > > >
> >> > > > > > > > > > > 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 <
> >> > > > > > >
> >> > > > > > > [hidden email]
> >> > > > > > > > > :
> >> > > > > > > > > > > > > >
> >> > > > > > > > > > > > > > > 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"
> >> > > > > > > > > > > > >
> >> > > > > > > > > > > > >
> >> > > > > > > > > >
> >> > > > > > > > > >
> >> > > > > > > > > >
> >> > > > > > > > > > --
> >> > > > > > > > > > Best regards,
> >> > > > > > > > > >   Andrey Kuznetsov.
> >> > > > > > > > > >
> >> > > >
> >> > > >
> >> > > >
> >> > > > --
> >> > > > Best regards,
> >> > > >   Andrey Kuznetsov.
> >> > > >
> >>
> >
> >
>