ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitry Pavlov <dpavlov....@gmail.com>
Subject Re: Ticket review checklist
Date Thu, 10 May 2018 13:15:57 GMT
Hi Ilya,

Error text `Unexpected exception during cache update` is brilliant example
how product should not behave.

So requiring contributors to explain reasons of failure would be good first
step to come to situation that exception text is so clear that user will
know what to do. It will defenetely reduce number of messages to user list.

Sincerely,
Dmitriy Pavlov


чт, 10 мая 2018 г. в 15:50, Ilya Kasnacheev <ilya.kasnacheev@gmail.com>:

> I don't think I quite understand how exception explanations should work.
>
> Imagine we have the following exception:
>
> // At least RuntimeException can be thrown by the code above when
> GridCacheContext is cleaned and there is
> // an attempt to use cleaned resources.
> U.error(log, "Unexpected exception during cache update", e);
>
> I mean, we genuinely don't know what happened here.
>
> Under new rules, what kind of "workaround" would that exception suggest?
> "Try turning it off and then back on"?
> What explanation how to resolve this exception can we offer? "Please write
> to dev@apache.ignite.org or to Apache JIRA, and then wait for a release
> with fix?"
>
> I'm really confused how we can implement 1.6 and 1.7 when dealing with
> messy real-world code.
>
> Regards,
>
>
> --
> Ilya Kasnacheev
>
> 2018-05-10 11:39 GMT+03:00 Vladimir Ozerov <vozerov@gridgain.com>:
>
> > Andrey, Anton, Alex
> >
> > Agree, *SHOULD* is more appropriate here.
> >
> > Please see latest version below. Does anyone want to add or change
> > something? Let's wait for several days for more feedback and then publish
> > and announce this list. Note that it would not be carved in stone and we
> > will be able to change it at any time if needed.
> >
> > 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 **SHOULD** 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.
> >
> >
> > On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov <stkuzma@gmail.com>
> > wrote:
> >
> > > Anton,
> > >
> > > I agree, *MUST* for exception reasons and *SHOULD* for ways of
> resolution
> > > sound clearer.
> > >
> > > 2018-05-08 12:56 GMT+03:00 Anton Vinogradov <av@apache.org>:
> > >
> > > > Andrey,
> > > >
> > > > How about
> > > > 1.6) All exceptions thrown to a user *MUST* have explanation of
> > > workaround
> > > > and contain original error.
> > > > All exceptions thrown to a user *SHOULD* have explanation how to
> > resolve
> > > if
> > > > possible.
> > > > ?
> > > >
> > > > вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov <stkuzma@gmail.com>:
> > > >
> > > > > Vladimir, checklist looks pleasant enough for me.
> > > > >
> > > > > I'd like to suggest one minor change. In 1.6 *MUST* seems to be too
> > > > strict,
> > > > > *SHOULD* would be enough. It can be frustrating for API user if I
> > > explain
> > > > > how to fix NPEs in a trivial way, for example.
> > > > >
> > > > > 2018-05-08 11:34 GMT+03:00 Anton Vinogradov <av@apache.org>:
> > > > >
> > > > > > Alex,
> > > > > >
> > > > > > It is not sounds like that, obviously.
> > > > > >
> > > > > > Tests should cover all negative and positive cases.
> > > > > > You should add enough tests to cover all cases.
> > > > > >
> > > > > > Sometimes one test can cover more than one case, so two tests
> *CAN*
> > > > > > partially check same things.
> > > > > > In case some cases already covered you should not create
> > duplicates.
> > > > > >
> > > > > > вт, 8 мая 2018 г. в 10:19, Александр Меньшиков <
> > sharplermc@gmail.com
> > > >:
> > > > > >
> > > > > > > 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.
> > > > > > > > > >> > > >
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Best regards,
> > > > >   Andrey Kuznetsov.
> > > > >
> > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > >   Andrey Kuznetsov.
> > >
> >
>

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