Anton, Checklist is not about checking, but about expectations and trust. Implementor should know what is expected from his patch. And reviewer is not inspector. His goal is to agree with implementer that checklist requirements s addressed. Consider thin client protocol compatibility. Without checklist implementer doesn't know about the requirement and does not address it. With checklist it is addressed. However, at the moment we do not have a framework to check clients compatibiltiy and it is rather hard to implement. So ideally implementer could demonstrate tests, but also he can just state "I tested it manually", which is also acceptable. This is where trust comes into play.
On Mon, May 7, 2018 at 6:45 PM, Anton Vinogradov <a...@apache.org> wrote: > Vova, > > Looks good to me. > > Please add clear explanation how to check 1.4, 1.5 and 2.x. > Also, this should be published as a wiki page with refs to... eg. Coding > Guidelines. > > пн, 7 мая 2018 г. в 17:26, Vladimir Ozerov <voze...@gridgain.com>: > > > Igniters, > > > > This is the checklist I have at the moment. Please let me know if you > have > > any comments on existing items, or want to add or remove anything. It > looks > > like we may have not only strict rules, but *nice to have* points here as > > well with help of *MUST*, *SHOULD* and *MAY* words as per RFC2119 [1]. So > > please feel free to suggest optional items as well. > > > > 1) API > > 1.1) API compatibility *MUST* be maintained between minor releases. Do > not > > remove existing methods or change their signatures, deprecate them > instead > > 1.2) Default behaviour "SHOULD NOT* be changed between minor releases, > > unless absolutely needed. If change is made, it *MUST* be described in > > "Migration Guide" > > 1.3) New operation *MUST* be well-documented in code (javadoc, > dotnetdoc): > > documentation must contain method's purpose, description of parameters > and > > how their values affect the outcome, description of return value and it's > > default, behavior in negative cases, interaction with other operations > and > > components > > 1.4) API parity between Java and .NET platforms *SHOULD* be maintained > when > > operation makes sense on both platforms. If method cannot be implemented > in > > a platform immediately, new JIRA ticket *MUST* be created and linked to > > current ticket > > 1.5) API parity between thin clients (Java, .NET) *SHOULD* be maintained > > when operation makes sense on several clients. If method cannot be > > implemented in a client immediately, new JIRA ticket *MUST* be created > and > > linked to current ticket > > 1.6) All exceptions thrown to a user *MUST* have explanation how to > > resolve, workaround or debug an error > > > > 2) Compatibility > > 2.1) Persistence backward compatibility *MUST* be maintained between > minor > > releases. It should be possible to start newer version on data files > > created by the previous version > > 2.2) Thin client forward and backward compatibility *SHOULD* be > maintained > > between two consecutive minor releases. If compatibility cannot be > > maintained it *MUST* be described in "Migration Guide" > > 2.3) JDBC and ODBC forward and backward compatibility *SHOULD* be > > maintained between two consecutive minor releases. If compatibility > cannot > > be maintained it *MUST* be described in "Migration Guide" > > > > 3) Tests > > 3.1) New functionality *MUST* be covered with unit tests for both > positive > > and negative use cases > > 3.2) All test suites *MUST* be run before merge to master..There *MUST* > be > > no new test failures > > > > 4) Code style *MUST* be followed as per Ignite's Coding Guidelines > > > > Vladimir. > > > > [1] https://www.ietf.org/rfc/rfc2119.txt > > > > On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov <voze...@gridgain.com> > > wrote: > > > > > Hi Dmitry, > > > > > > Yes, I'll do that in the nearest days. > > > > > > On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <dpavlov....@gmail.com> > > > wrote: > > > > > >> Igniters, the idea was related to small refactorings co-located with > > main > > >> change. > > >> > > >> Main change itself indicates that existing code did not meet the > > criteria > > >> of practice. Approving of standalone refactorings instead contradicts > > with > > >> principle don't touch if it works. So I still like idea of co-located > > >> changes improving code, javadocs, style, etc. > > >> > > >> But let's not argue about this point now, let's summarize the > undisputed > > >> points and add it to the wiki. Vladimir, would you please do it? > > >> > > >> > > >> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov <nizhi...@apache.org>: > > >> > > >> > Igniters, > > >> > > > >> > I agree with Vova. > > >> > > > >> > Don't fix if it works! > > >> > > > >> > If you 100% sure then it a useful addition to the product - just > make > > a > > >> > separate ticket. > > >> > > > >> > В Ср, 25/04/2018 в 11:44 +0300, Vladimir Ozerov пишет: > > >> > > Guys, > > >> > > > > >> > > The problem with in-place refactorings is that you increase > affected > > >> > scope. > > >> > > It is not uncommon to break compatibility or public contracts with > > >> even > > >> > > minor things. E.g. recently we decided drop org.jsr166 package in > > >> favor > > >> > of > > >> > > Java 8 classes. Innocent change. Result - broken storage. Another > > >> problem > > >> > > is conflicts. It is not uncommon to have long-lived branches which > > we > > >> > need > > >> > > to merge with master over and over again. And a lot of > refactorings > > >> cause > > >> > > conflicts. It is much easier to resolve them if you know that > logic > > >> was > > >> > not > > >> > > affected as opposed to cases when you need to resolve both renames > > and > > >> > > method extractions along with business-logic changes. > > >> > > > > >> > > I'd like to repeat - if you have a time for refactoring then you > > >> > definitely > > >> > > have a time to extract these changes to separate PR and submit a > > >> separate > > >> > > ticket. I am quite understand what "low priority" do you mean if > you > > >> do > > >> > > refactorings on your own. > > >> > > > > >> > > On Tue, Apr 24, 2018 at 10:52 PM, Andrey Kuznetsov < > > stku...@gmail.com > > >> > > > >> > > wrote: > > >> > > > > >> > > > +1. > > >> > > > > > >> > > > Once again, I beg for "small refactoring permission" in a > > checklist. > > >> > As of > > >> > > > today, separate tickets for small refactorings has lowest > > priority, > > >> > since > > >> > > > they neither fix any flaw nor add new functionality. Also, the > > >> > attempts to > > >> > > > make issue-related code safer / cleaner / more readable in > "real" > > >> pull > > >> > > > requests are typically rejected, since they contradict our > current > > >> > > > guidelines. > > >> > > > > > >> > > > I understand this will require a bit more effort from > > >> > committer/maintainer, > > >> > > > but otherwise we will get constantly degrading code quality. > > >> > > > > > >> > > > > > >> > > > 2018-04-24 18:52 GMT+03:00 Eduard Shangareev < > > >> > eduard.shangar...@gmail.com > > >> > > > > : > > >> > > > > Vladimir, > > >> > > > > > > >> > > > > I am not talking about massive/sophisticated refactoring. But > I > > >> > believe > > >> > > > > that ask to extract some methods should be OK to do without an > > >> extra > > >> > > > > ticket. > > >> > > > > > > >> > > > > A checklist shouldn't be necessarily a set of certain rules > but > > >> also > > >> > it > > >> > > > > could include suggestion and reminders. > > >> > > > > > > >> > > > > On Tue, Apr 24, 2018 at 6:39 PM, Vladimir Ozerov < > > >> > voze...@gridgain.com> > > >> > > > > wrote: > > >> > > > > > > >> > > > > > Ed, > > >> > > > > > > > >> > > > > > Refactoring is a separate task. If you would like to rework > > >> > exchange > > >> > > > > > > >> > > > > future > > >> > > > > > - please do this in a ticket "Refactor exchange task", > nobody > > >> would > > >> > > > > > > >> > > > > against > > >> > > > > > this. This is just a matter of creating separate ticket and > > >> > separate > > >> > > > > > >> > > > PR. > > >> > > > > If > > >> > > > > > one have a time for refactoring, it should not be a problem > > for > > >> > him to > > >> > > > > > spend several minutes on JIRA and GitHub. > > >> > > > > > > > >> > > > > > As far as documentation - what you describe is normal review > > >> > process, > > >> > > > > > > >> > > > > when > > >> > > > > > reviewer might want to ask contributor to fix something. > > >> Checklist > > >> > is a > > >> > > > > > different thing - this is a set of rules which must be > > followed > > >> by > > >> > > > > > > >> > > > > anyone. > > >> > > > > > I do not understand how you can define documentation in this > > >> > checklist. > > >> > > > > > Same problem with logging - what is "enough"? > > >> > > > > > > > >> > > > > > On Tue, Apr 24, 2018 at 4:51 PM, Eduard Shangareev < > > >> > > > > > eduard.shangar...@gmail.com> wrote: > > >> > > > > > > > >> > > > > > > Igniters, > > >> > > > > > > > > >> > > > > > > I don't understand why you are so against refactoring. > > >> > > > > > > Code already smells like hell. Methods 200+ line is > normal. > > >> > Exchange > > >> > > > > > > > >> > > > > > future > > >> > > > > > > is asking to be separated on several one. Transaction code > > >> could > > >> > > > > > > > >> > > > > > understand > > >> > > > > > > few people. > > >> > > > > > > > > >> > > > > > > If we separate refactoring from development it would mean > > that > > >> > no one > > >> > > > > > > > >> > > > > > will > > >> > > > > > > do it. > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > 2) Documentation. > > >> > > > > > > Everything which was asked by reviewers to clarify idea > > >> should be > > >> > > > > > > > >> > > > > > reflected > > >> > > > > > > in the code. > > >> > > > > > > > > >> > > > > > > 3) Logging. > > >> > > > > > > Logging should be enough to troubleshoot the problem if > > >> someone > > >> > comes > > >> > > > > > > >> > > > > to > > >> > > > > > > user-list with an issue in the code. > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > On Fri, Apr 20, 2018 at 7:06 PM, Dmitry Pavlov < > > >> > > > > > >> > > > dpavlov....@gmail.com> > > >> > > > > > > wrote: > > >> > > > > > > > > >> > > > > > > > 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, Александр Меньшиков < > > >> > > > > > > >> > > > > sharple...@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 < > > >> > stku...@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 < > > >> > > > > > >> > > > voze...@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.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" > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > -- > > >> > > > > > > > > > Best regards, > > >> > > > > > > > > > Andrey Kuznetsov. > > >> > > > > > > > > > > > >> > > > > > >> > > > > > >> > > > > > >> > > > -- > > >> > > > Best regards, > > >> > > > Andrey Kuznetsov. > > >> > > > > > >> > > > > > > > > >
