ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Kuznetsov <stku...@gmail.com>
Subject Re: Ticket review checklist
Date Fri, 20 Apr 2018 13:54:43 GMT
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