ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitry Pavlov <dpavlov....@gmail.com>
Subject Re: Ticket review checklist
Date Fri, 20 Apr 2018 16:06:39 GMT
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, Александр Меньшиков <sharplermc@gmail.com>:

> 4) Metrics.
> partially +1
>
> It makes sense to have some minimal code coverage for new code in PR. IMHO.
>
> Also, we can limit the cyclomatic complexity of the new code in PR too.
>
> 6) Refactoring
> -1
>
> I understand why people want to refactor old code.
> But I think refactoring should be always a separate task.
> And it's better to remove all refactoring from PR, if it's not the sense of
> the issue.
>
>
> 2018-04-20 16:54 GMT+03:00 Andrey Kuznetsov <stkuzma@gmail.com>:
>
> > 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 <vozerov@gridgain.com>:
> >
> > > Hi Ed,
> > >
> > > Unfortunately some of these points are not good candidates for the
> > > checklist because of these:
> > > - It must be clear and disallow *multiple interpretations*
> > > - It must be *lightweight*, otherwise Ignite development would become a
> > > nightmare
> > >
> > > We cannot have "nice to have" points here. Checklist should answer the
> > > question "is ticket eligible to be merged?"
> > >
> > > >>> 1) Code style.
> > > +1
> > >
> > > >>>  2) Documentation
> > > -1, it is impossible to define what is "well-documented". A piece of
> code
> > > could be obvious for one contributor, and non-obvious for another. In
> any
> > > case this is not a blocker for merge. Instead, during review one can
> ask
> > > implementer to add more docs, but it cannot be forced.
> > >
> > > >>>  3) Logging
> > > -1, same problem - what is "enough logging?". Enough for whom? How to
> > > understand whether it is enough or not?
> > >
> > > >>>  4) Metrics
> > > -1, no clear boundaries, and decision on whether metrics are to be
> added
> > or
> > > not should be performed during design phase. As before, it is perfectly
> > > valid to ask contributor to add metrics with clear explanation why, but
> > > this is not part of the checklist.
> > >
> > > >>> 5) TC status
> > > +1, already mentioned
> > >
> > > >>>  6) Refactoring
> > > Strong -1. OOP is a slippery slope, there are no good and bad receipts
> > for
> > > all cases, hence it cannot be used in a checklist.
> > >
> > > We can borrow useful rules from p.2, p.3 and p.4 if you provide clear
> > > definitions on how to measure them.
> > >
> > > Vladimir.
> > >
> > > On Fri, Apr 20, 2018 at 3:50 PM, Eduard Shangareev <
> > > eduard.shangareev@gmail.com> wrote:
> > >
> > > > Also, I want to add some technical requirement. Let's discuss them.
> > > >
> > > > 1) Code style.
> > > > The code needs to be formatted according to coding guidelines
> > > > <
> https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines
> > >.
> > > > The
> > > > code must not contain TODOs without a ticket reference.
> > > >
> > > > It is highly recommended to make major formatting changes in existing
> > > code
> > > > as a separate commit, to make review process more practical.
> > > >
> > > > 2) Documentation.
> > > > Added code should be well-documented. Any methods that raise
> questions
> > > > regarding their code flow, invariants, synchronization, etc., must be
> > > > documented with comprehensive javadoc. Any reviewer can request that
> a
> > > > particular added method be documented. Also, it is a good practice to
> > > > document old code in a 10-20 lines region around changed code.
> > > >
> > > > 3) Logging.
> > > > Make sure that there are enough logging added in every category for
> > > > possible diagnostic in field. Check that logging messages are
> properly
> > > > spelled.
> > > >
> > > > 4) Metrics.
> > > > Are there any metrics that need to be exposed to user?
> > > >
> > > > 5) TC status.
> > > >
> > > > Recheck that there are no new failing tests
> > > >
> > > > 6) Refactoring.
> > > >
> > > > The code should be better than before:
> > > >
> > > >    - extract method from big one;
> > > >    - do anything else to make code clearer (don't forget about some
> > > >    OOP-practise, replace if-else hell with inheritance
> > > >    - split refactoring (renaming, code format) from actual changes by
> > > >    separate commit
> > > >
> > > >
> > > >
> > > >
> > > > On Fri, Apr 20, 2018 at 3:23 PM, Eduard Shangareev <
> > > > eduard.shangareev@gmail.com> wrote:
> > > >
> > > > > Hi, guys.
> > > > >
> > > > > I believe that we should update maintainers list before adding this
> > > > review
> > > > > requirement.
> > > > > There should not be the situation when there is only one
> contributor
> > > who
> > > > > is responsible for a component.
> > > > > We already have issues with review speed and response time.
> > > > >
> > > > > On Fri, Apr 20, 2018 at 2:17 PM, Anton Vinogradov <av@apache.org>
> > > 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 <nizhikov@apache.org>:
> > > > >>
> > > > >> > 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.
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message