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