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