Vladimir, the 3.1 is a bit unclear for me. Which code coverage is acceptable? Now it sounds like two tests are enough (one for positive and one for negative cases).
2018-05-07 23:09 GMT+03:00 Dmitriy Setrakyan <dsetrak...@apache.org>: > Is this list on the Wiki? > > On Mon, May 7, 2018 at 7:26 AM, Vladimir Ozerov <voze...@gridgain.com> > wrote: > > > 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. > > >> > > > > > >> > > > > > > > > >
