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. > >
signature.asc
Description: This is a digitally signed message part
