ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vladimir Ozerov <voze...@gridgain.com>
Subject Re: Ticket review checklist
Date Wed, 23 May 2018 14:02:03 GMT
Igniters,

I created review checklist on WIKI [1] and also fixed related pages (e.g.
"How To Contribute"). Please let me know if you have any comments before I
go with public announce.

Vladimir.

[1] https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist

On Thu, May 10, 2018 at 5:10 PM, Vladimir Ozerov <vozerov@gridgain.com>
wrote:

> Ilya,
>
> We define that exception messages *SHOULD* have clear explanation on what
> is wrong. *SHOULD* mean that the rule should be followed unless there is a
> reason not to follow. In your case you refer to some unexpected behavior.
> I.e. an exceptional situation developer is not aware of. In this case for
> sure we cannot force contributor to explain what is wrong, because, well,
> we don't know. This is why we relaxed the rule from *MUST* to *SHOULD*.
>
> On Thu, May 10, 2018 at 3:50 PM, Ilya Kasnacheev <
> ilya.kasnacheev@gmail.com> wrote:
>
>> 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