From dev-return-33708-archive-asf-public=cust-asf.ponee.io@ignite.apache.org Fri Apr 20 15:54:51 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 0A84F180645 for ; Fri, 20 Apr 2018 15:54:50 +0200 (CEST) Received: (qmail 92027 invoked by uid 500); 20 Apr 2018 13:54:50 -0000 Mailing-List: contact dev-help@ignite.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@ignite.apache.org Delivered-To: mailing list dev@ignite.apache.org Received: (qmail 92011 invoked by uid 99); 20 Apr 2018 13:54:49 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd2-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Fri, 20 Apr 2018 13:54:49 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd2-us-west.apache.org (ASF Mail Server at spamd2-us-west.apache.org) with ESMTP id DC8631A01FE for ; Fri, 20 Apr 2018 13:54:48 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd2-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 1.899 X-Spam-Level: * X-Spam-Status: No, score=1.899 tagged_above=-999 required=6.31 tests=[DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_MESSAGE=2, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H2=-0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=disabled Authentication-Results: spamd2-us-west.apache.org (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com Received: from mx1-lw-eu.apache.org ([10.40.0.8]) by localhost (spamd2-us-west.apache.org [10.40.0.9]) (amavisd-new, port 10024) with ESMTP id xVZ5TQ69wtqa for ; Fri, 20 Apr 2018 13:54:46 +0000 (UTC) Received: from mail-io0-f174.google.com (mail-io0-f174.google.com [209.85.223.174]) by mx1-lw-eu.apache.org (ASF Mail Server at mx1-lw-eu.apache.org) with ESMTPS id 9B2A25F24C for ; Fri, 20 Apr 2018 13:54:45 +0000 (UTC) Received: by mail-io0-f174.google.com with SMTP id d26-v6so10646741ioc.2 for ; Fri, 20 Apr 2018 06:54:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to; bh=hZ/h0XEJnReC1bTYot5F2fNXIBPj9/jTF/KhM6i5QaY=; b=oObKdpqAJ8Hk9PowO/fXOOHz2Hnqz2AY4tF+dwPWSNz9qJB5Vl74js+SXWdPBDs1lV vUoBBipNqNgxQS5hC6J/EdkXJRJJ6E+LRi28zCajGov0CVjEU4yj8ZOiLsFqVuoq0/Ml IiTJsT2nRXca21yLxGIYABMDtNte/BXNp7z5YEvfcWzKyhzNqiV5F5IYALRfwpYTd4Jo Y1tcO7rAYr0AqkvsZyorwyHmtq2GK4VeoST8/8femIb1el+/gzwVogBQheuBFCqaejTF Ip09sMNxheosFj6SuGOFDwMFcn6C0C2CA2FIQ+xm64ugXrSR/z00vchlYH3b7YEImjb9 Ax2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=hZ/h0XEJnReC1bTYot5F2fNXIBPj9/jTF/KhM6i5QaY=; b=U4rrclWacAHEhnUbyu4b35a+UFgMztbtClk5UlBhArthwOi0DVePlbPz13mZM+s9AZ y6MidVGqtETeYtorcHvQ82RAgHrZO8eWsJo53K2bA80PDwYIOgHQZCYOyUo0dJP7Ibkx RAjsqsoW9Givas6G5yW6OeQqdzrb6liVrovOZXpgd+rYt+LvOXBtvAuPGs79M33+P5ii ckPY+SJdy8JlZ769QvE1bT25H58jv8OR9wHj+lFcyqEJodJxSI00mHZo1Wz64azu4WRd m+Kwrp7609NEhl5ktvhOlAgLRerJQttVOtnG7mx1lI8yLsIyMb0MdtdEYc8j9HvUHtui FqRw== X-Gm-Message-State: ALQs6tDYSmiCbMgbHqvW3naY0nsegvSkc9FL87s2ZphxEYeBjVxLVD6x FOHevnjdC35dY8yRSYsBuiftIhsm0cAXar8EP+s= X-Google-Smtp-Source: AIpwx49GkB52WKppSizs8WKDL2q/HsNEDEYE/YlnciUi2e8rxrdFpWRnfnBh7EacxZdllsrlDwnfo0qa8dw+rJa+XKY= X-Received: by 2002:a6b:bd4:: with SMTP id 81-v6mr10551717iol.25.1524232484086; Fri, 20 Apr 2018 06:54:44 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:4406:0:0:0:0:0 with HTTP; Fri, 20 Apr 2018 06:54:43 -0700 (PDT) In-Reply-To: References: <1524174823.26141.76.camel@gmail.com> From: Andrey Kuznetsov Date: Fri, 20 Apr 2018 16:54:43 +0300 Message-ID: Subject: Re: Ticket review checklist To: dev@ignite.apache.org Content-Type: multipart/alternative; boundary="000000000000c4e924056a480757" --000000000000c4e924056a480757 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 : > 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 fo= r > 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 > > . > > 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 > wrote: > > > > > >> Vova, > > >> > > >> Everything you described sound good to me. > > >> > > >> I'd like to propose to create special page at AI Wiki and to describ= e > > >> 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 : > > >> > > >> > 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 t= wo > > >> > 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 > > >> > > > >> > > > >> > =D0=92 =D0=A7=D1=82, 19/04/2018 =D0=B2 23:00 +0300, Vladimir Ozero= v =D0=BF=D0=B8=D1=88=D0=B5=D1=82: > > >> > > 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 n= ot > > >> > > sufficient* phase before commit could be added to the main branc= h. > > 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 agre= e > 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 b= y > > >> > > component's maintainer* > > >> > > 3) If ticket changes public API or existing behavior, at least t= wo > > >> > > 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" > > >> > > > >> > > > > > > > > > --=20 Best regards, Andrey Kuznetsov. --000000000000c4e924056a480757--