ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ilya Kasnacheev <ilya.kasnach...@gmail.com>
Subject Re: Ticket review checklist
Date Thu, 10 May 2018 13:22:50 GMT
Dmitry,

can you please perform an excercise of looking that piece of code up
(org.apache.ignite.internal.processors.cache.distributed.dht.atomic.GridDhtAtomicCache),
telling us how an ideal message should look like in this case?

Thanks,

-- 
Ilya Kasnacheev

2018-05-10 16:15 GMT+03:00 Dmitry Pavlov <dpavlov.spb@gmail.com>:

> 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