ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Kuznetsov <stku...@gmail.com>
Subject Re: Ticket review checklist
Date Tue, 08 May 2018 10:05:13 GMT
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