ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Александр Меньшиков <sharple...@gmail.com>
Subject Re: Ticket review checklist
Date Tue, 08 May 2018 07:19:08 GMT
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 <dsetrakyan@apache.org>:

> Is this list on the Wiki?
>
> On Mon, May 7, 2018 at 7:26 AM, Vladimir Ozerov <vozerov@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 <vozerov@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.spb@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 <nizhikov@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 <
> > stkuzma@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.shangareev@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 <
> > >> > vozerov@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.shangareev@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.spb@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, Александр Меньшиков <
> > >> > > > >
> > >> > > > > sharplermc@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 <
> > >> > stkuzma@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 <
> > >> > > >
> > >> > > > vozerov@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.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
> > >> > > > > > > > > > > > <
> > >> > > > > > > > >
> > >> > > > > > > > > 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.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
> > >> <
> > >> > > > > > >
> > >> > > > > > > av@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 <
> > >> > > > > > >
> > >> > > > > > > nizhikov@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.
> > >> > > >
> > >>
> > >
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message