ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitry Pavlov <dpavlov....@gmail.com>
Subject Re: Ticket review checklist
Date Mon, 04 Jun 2018 11:52:59 GMT
Vladimir, I mean strict definition, how much previous runs should
contributor consider? What if test was failed by infrastructure reason in
master previously, how can contributor be sure test failure != broken code
in PR? In this case it should be double checked by contributor/reviewer.
I'm sure nobody can give strict definition of 'new' failure.

Flaky tests detected by TC may be taken into account in check-list, because
contributor can check if failure is flaky. But again, not all tests with
floating failure is detected by TC as flaky.

I don't understand what problem will be solved if we soften current
requirement with 'new' test? Everybody will continue to complain they PR's
test failures is not `new`. So let's keep it as is.

пн, 4 июн. 2018 г. в 14:46, Vladimir Ozerov <vozerov@gridgain.com>:

> Dmitry,
>
> New failure is a failure hasn't happened on previous runs. If it do
> happened, then contributor should see if it is a flaky or not through local
> and TC runs. The same works for timeout suites.
> Current statement in "Review Checklist" that there are should be no failed
> tests is not applicable to real word. Almost every patch is pushed to
> repository with test failures.
>
> On Mon, Jun 4, 2018 at 2:22 PM, Dmitry Pavlov <dpavlov.spb@gmail.com>
> wrote:
>
> > Hi Vladimir, could you provide definition what is new failure? how do you
> > know it is new or not?
> >
> > And please forget for a moment you're Ignite expert & veteran, imagine
> you
> > are newcomer.
> >
> > I can't find any criteria that can be used by newbie to come to the
> > conclusion that test is new. Patch is accepted by reviewer, so it should
> be
> > up to him to correctly register failures in tickets with
> > MakeTeamCityGreenAgain label and mute unimportant tests.
> >
> > пн, 4 июн. 2018 г. в 11:32, Vladimir Ozerov <vozerov@gridgain.com>:
> >
> > > Dmitry,
> > >
> > > I still do not see how new patches could be accepted with this
> > requirement
> > > in place. Consider the following case: I created a patch and run it on
> > TC,
> > > observed N failures, verified through TC history that none if them are
> > new.
> > > Am I eligible to push the commit?
> > >
> > > On Thu, May 24, 2018 at 3:11 PM, Dmitry Pavlov <dpavlov.spb@gmail.com>
> > > wrote:
> > >
> > > > Petr, good point. It is more intuitive, we should mark test we can
> > ignore
> > > > by mute.
> > > >
> > > > So Vladimir, you or other Ignite veteran can mute test, if can say it
> > is
> > > > not important.
> > > >
> > > > чт, 24 мая 2018 г. в 15:07, Petr Ivanov <mr.weider@gmail.com>:
> > > >
> > > > > Why cannot we mute (and file corresponding tickets) all test
> failures
> > > > > (including flaky) to some date and start initiative Green TC?
> > > > >
> > > > >
> > > > >
> > > > > > On 24 May 2018, at 15:04, Vladimir Ozerov <vozerov@gridgain.com>
> > > > wrote:
> > > > > >
> > > > > > Dmitry,
> > > > > >
> > > > > > We cannot add this requirements, because we do have failures on
> TC.
> > > > This
> > > > > > requirement implies that all development would stop until TC is
> > > green.
> > > > > > We never had old requirement work, neither we need to enforce it
> > now.
> > > > > >
> > > > > > On Thu, May 24, 2018 at 2:59 PM, Dmitry Pavlov <
> > > dpavlov.spb@gmail.com>
> > > > > > wrote:
> > > > > >
> > > > > >> 3.c
> > > > > >>
> > > > > >>   1. All test suites *MUST* be run on TeamCity [3] before merge
> to
> > > > > master,
> > > > > >>   there *MUST NOT* be any test failures
> > > > > >>
> > > > > >>
> > > > > >> 'New' word should be removed because we cant separate `new` and
> > `non
> > > > > new`
> > > > > >> failures.
> > > > > >>
> > > > > >> Let's imagine example, we have 50 green runs in master. And PR
> > > Run-All
> > > > > >> contains this test failed. Is it new or not new? Actually we
> don't
> > > > know.
> > > > > >>
> > > > > >> Existing requirement is about all TC must be green, so let's
> keep
> > it
> > > > as
> > > > > is.
> > > > > >>
> > > > > >> ср, 23 мая 2018 г. в 17:02, Vladimir Ozerov <
> vozerov@gridgain.com
> > >:
> > > > > >>
> > > > > >>> Igniters,
> > > > > >>>
> > > > > >>> I created review checklist on WIKI [1] and also fixed related
> > pages
> > > > > (e.g.
> > > > > >>> "How To Contribute"). Please let me know if you have any
> comments
> > > > > before
> > > > > >> I
> > > > > >>> go with public announce.
> > > > > >>>
> > > > > >>> Vladimir.
> > > > > >>>
> > > > > >>> [1]
> > > > >
> https://cwiki.apache.org/confluence/display/IGNITE/Review+Checklist
> > > > > >>>
> > > > > >>> On Thu, May 10, 2018 at 5:10 PM, Vladimir Ozerov <
> > > > vozerov@gridgain.com
> > > > > >
> > > > > >>> wrote:
> > > > > >>>
> > > > > >>>> Ilya,
> > > > > >>>>
> > > > > >>>> We define that exception messages *SHOULD* have clear
> > explanation
> > > on
> > > > > >> what
> > > > > >>>> is wrong. *SHOULD* mean that the rule should be followed
> unless
> > > > there
> > > > > >> is
> > > > > >>> a
> > > > > >>>> reason not to follow. In your case you refer to some
> unexpected
> > > > > >> behavior.
> > > > > >>>> I.e. an exceptional situation developer is not aware of. In
> this
> > > > case
> > > > > >> for
> > > > > >>>> sure we cannot force contributor to explain what is wrong,
> > > because,
> > > > > >> well,
> > > > > >>>> we don't know. This is why we relaxed the rule from *MUST* to
> > > > > *SHOULD*.
> > > > > >>>>
> > > > > >>>> On Thu, May 10, 2018 at 3:50 PM, Ilya Kasnacheev <
> > > > > >>>> ilya.kasnacheev@gmail.com> wrote:
> > > > > >>>>
> > > > > >>>>> I don't think I quite understand how exception explanations
> > > should
> > > > > >> work.
> > > > > >>>>>
> > > > > >>>>> Imagine we have the following exception:
> > > > > >>>>>
> > > > > >>>>> // At least RuntimeException can be thrown by the code above
> > when
> > > > > >>>>> GridCacheContext is cleaned and there is
> > > > > >>>>> // an attempt to use cleaned resources.
> > > > > >>>>> U.error(log, "Unexpected exception during cache update", e);
> > > > > >>>>>
> > > > > >>>>> I mean, we genuinely don't know what happened here.
> > > > > >>>>>
> > > > > >>>>> Under new rules, what kind of "workaround" would that
> exception
> > > > > >> suggest?
> > > > > >>>>> "Try turning it off and then back on"?
> > > > > >>>>> What explanation how to resolve this exception can we offer?
> > > > "Please
> > > > > >>> write
> > > > > >>>>> to dev@apache.ignite.org or to Apache JIRA, and then wait
> for
> > a
> > > > > >> release
> > > > > >>>>> with fix?"
> > > > > >>>>>
> > > > > >>>>> I'm really confused how we can implement 1.6 and 1.7 when
> > dealing
> > > > > with
> > > > > >>>>> messy real-world code.
> > > > > >>>>>
> > > > > >>>>> Regards,
> > > > > >>>>>
> > > > > >>>>>
> > > > > >>>>> --
> > > > > >>>>> Ilya Kasnacheev
> > > > > >>>>>
> > > > > >>>>> 2018-05-10 11:39 GMT+03:00 Vladimir Ozerov <
> > vozerov@gridgain.com
> > > >:
> > > > > >>>>>
> > > > > >>>>>> Andrey, Anton, Alex
> > > > > >>>>>>
> > > > > >>>>>> Agree, *SHOULD* is more appropriate here.
> > > > > >>>>>>
> > > > > >>>>>> Please see latest version below. Does anyone want to add or
> > > change
> > > > > >>>>>> something? Let's wait for several days for more feedback and
> > > then
> > > > > >>>>> publish
> > > > > >>>>>> and announce this list. Note that it would not be carved in
> > > stone
> > > > > >> and
> > > > > >>> we
> > > > > >>>>>> will be able to change it at any time if needed.
> > > > > >>>>>>
> > > > > >>>>>> 1) API
> > > > > >>>>>> 1.1) API compatibility *MUST* be maintained between minor
> > > > releases.
> > > > > >> Do
> > > > > >>>>> not
> > > > > >>>>>> remove existing methods or change their signatures,
> deprecate
> > > them
> > > > > >>>>> instead
> > > > > >>>>>> 1.2) Default behaviour "SHOULD NOT* be changed between minor
> > > > > >> releases,
> > > > > >>>>>> unless absolutely needed. If change is made, it *MUST* be
> > > > described
> > > > > >> in
> > > > > >>>>>> "Migration Guide"
> > > > > >>>>>> 1.3) New operation *MUST* be well-documented in code
> (javadoc,
> > > > > >>>>> dotnetdoc):
> > > > > >>>>>> documentation must contain method's purpose, description of
> > > > > >> parameters
> > > > > >>>>> and
> > > > > >>>>>> how their values affect the outcome, description of return
> > value
> > > > and
> > > > > >>>>> it's
> > > > > >>>>>> default, behavior in negative cases, interaction with other
> > > > > >> operations
> > > > > >>>>> and
> > > > > >>>>>> components
> > > > > >>>>>> 1.4) API parity between Java and .NET platforms *SHOULD* be
> > > > > >> maintained
> > > > > >>>>> when
> > > > > >>>>>> operation makes sense on both platforms. If method cannot be
> > > > > >>>>> implemented in
> > > > > >>>>>> a platform immediately, new JIRA ticket *MUST* be created
> and
> > > > linked
> > > > > >>> to
> > > > > >>>>>> current ticket
> > > > > >>>>>> 1.5) API parity between thin clients (Java, .NET) *SHOULD*
> be
> > > > > >>> maintained
> > > > > >>>>>> when operation makes sense on several clients. If method
> > cannot
> > > be
> > > > > >>>>>> implemented in a client immediately, new JIRA ticket *MUST*
> be
> > > > > >> created
> > > > > >>>>> and
> > > > > >>>>>> linked to current ticket
> > > > > >>>>>> 1.6) All exceptions thrown to a user **SHOULD** have
> > explanation
> > > > how
> > > > > >>> to
> > > > > >>>>>> resolve, workaround or debug an error
> > > > > >>>>>>
> > > > > >>>>>> 2) Compatibility
> > > > > >>>>>> 2.1) Persistence backward compatibility *MUST* be maintained
> > > > between
> > > > > >>>>> minor
> > > > > >>>>>> releases. It should be possible to start newer version on
> data
> > > > files
> > > > > >>>>>> created by the previous version
> > > > > >>>>>> 2.2) Thin client forward and backward compatibility *SHOULD*
> > be
> > > > > >>>>> maintained
> > > > > >>>>>> between two consecutive minor releases. If compatibility
> > cannot
> > > be
> > > > > >>>>>> maintained it *MUST* be described in "Migration Guide"
> > > > > >>>>>> 2.3) JDBC and ODBC forward and backward compatibility
> *SHOULD*
> > > be
> > > > > >>>>>> maintained between two consecutive minor releases. If
> > > > compatibility
> > > > > >>>>> cannot
> > > > > >>>>>> be maintained it *MUST* be described in "Migration Guide"
> > > > > >>>>>>
> > > > > >>>>>> 3) Tests
> > > > > >>>>>> 3.1) New functionality *MUST* be covered with unit tests for
> > > both
> > > > > >>>>> positive
> > > > > >>>>>> and negative use cases
> > > > > >>>>>> 3.2) All test suites *MUST* be run before merge to
> > master..There
> > > > > >>> *MUST*
> > > > > >>>>> be
> > > > > >>>>>> no new test failures
> > > > > >>>>>>
> > > > > >>>>>> 4) Code style *MUST* be followed as per Ignite's Coding
> > > Guidelines
> > > > > >>>>>>
> > > > > >>>>>> Vladimir.
> > > > > >>>>>>
> > > > > >>>>>>
> > > > > >>>>>> On Tue, May 8, 2018 at 1:05 PM, Andrey Kuznetsov <
> > > > stkuzma@gmail.com
> > > > > >>>
> > > > > >>>>>> wrote:
> > > > > >>>>>>
> > > > > >>>>>>> Anton,
> > > > > >>>>>>>
> > > > > >>>>>>> I agree, *MUST* for exception reasons and *SHOULD* for ways
> > of
> > > > > >>>>> resolution
> > > > > >>>>>>> sound clearer.
> > > > > >>>>>>>
> > > > > >>>>>>> 2018-05-08 12:56 GMT+03:00 Anton Vinogradov <av@apache.org
> >:
> > > > > >>>>>>>
> > > > > >>>>>>>> Andrey,
> > > > > >>>>>>>>
> > > > > >>>>>>>> How about
> > > > > >>>>>>>> 1.6) All exceptions thrown to a user *MUST* have
> explanation
> > > of
> > > > > >>>>>>> workaround
> > > > > >>>>>>>> and contain original error.
> > > > > >>>>>>>> All exceptions thrown to a user *SHOULD* have explanation
> > how
> > > to
> > > > > >>>>>> resolve
> > > > > >>>>>>> if
> > > > > >>>>>>>> possible.
> > > > > >>>>>>>> ?
> > > > > >>>>>>>>
> > > > > >>>>>>>> вт, 8 мая 2018 г. в 12:26, Andrey Kuznetsov <
> > > stkuzma@gmail.com
> > > > > >>> :
> > > > > >>>>>>>>
> > > > > >>>>>>>>> Vladimir, checklist looks pleasant enough for me.
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> I'd like to suggest one minor change. In 1.6 *MUST* seems
> > to
> > > > > >> be
> > > > > >>>>> too
> > > > > >>>>>>>> strict,
> > > > > >>>>>>>>> *SHOULD* would be enough. It can be frustrating for API
> > user
> > > > > >> if
> > > > > >>> I
> > > > > >>>>>>> explain
> > > > > >>>>>>>>> how to fix NPEs in a trivial way, for example.
> > > > > >>>>>>>>>
> > > > > >>>>>>>>> 2018-05-08 11:34 GMT+03:00 Anton Vinogradov <
> av@apache.org
> > >:
> > > > > >>>>>>>>>
> > > > > >>>>>>>>>> Alex,
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> It is not sounds like that, obviously.
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> Tests should cover all negative and positive cases.
> > > > > >>>>>>>>>> You should add enough tests to cover all cases.
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> Sometimes one test can cover more than one case, so two
> > > > > >> tests
> > > > > >>>>> *CAN*
> > > > > >>>>>>>>>> partially check same things.
> > > > > >>>>>>>>>> In case some cases already covered you should not create
> > > > > >>>>>> duplicates.
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>> вт, 8 мая 2018 г. в 10:19, Александр Меньшиков <
> > > > > >>>>>> sharplermc@gmail.com
> > > > > >>>>>>>> :
> > > > > >>>>>>>>>>
> > > > > >>>>>>>>>>> Vladimir, the 3.1 is a bit unclear for me. Which code
> > > > > >>>>> coverage is
> > > > > >>>>>>>>>>> acceptable? Now it sounds like two tests are enough
> (one
> > > > > >> for
> > > > > >>>>>>> positive
> > > > > >>>>>>>>> and
> > > > > >>>>>>>>>>> one for negative cases).
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>> 2018-05-07 23:09 GMT+03:00 Dmitriy Setrakyan <
> > > > > >>>>>>> dsetrakyan@apache.org
> > > > > >>>>>>>>> :
> > > > > >>>>>>>>>>>
> > > > > >>>>>>>>>>>> Is this list on the Wiki?
> > > > > >>>>>>>>>>>>
> > > > > >>>>>>>>>>>> On Mon, May 7, 2018 at 7:26 AM, Vladimir Ozerov <
> > > > > >>>>>>>>> vozerov@gridgain.com>
> > > > > >>>>>>>>>>>> wrote:
> > > > > >>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> Igniters,
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> This is the checklist I have at the moment. Please
> let
> > > > > >>> me
> > > > > >>>>>> know
> > > > > >>>>>>> if
> > > > > >>>>>>>>> you
> > > > > >>>>>>>>>>>> have
> > > > > >>>>>>>>>>>>> any comments on existing items, or want to add or
> > > > > >> remove
> > > > > >>>>>>>> anything.
> > > > > >>>>>>>>> It
> > > > > >>>>>>>>>>>> looks
> > > > > >>>>>>>>>>>>> like we may have not only strict rules, but *nice to
> > > > > >>> have*
> > > > > >>>>>>> points
> > > > > >>>>>>>>>> here
> > > > > >>>>>>>>>>> as
> > > > > >>>>>>>>>>>>> well with help of *MUST*, *SHOULD* and *MAY* words as
> > > > > >>> per
> > > > > >>>>>>> RFC2119
> > > > > >>>>>>>>>> [1].
> > > > > >>>>>>>>>>> So
> > > > > >>>>>>>>>>>>> please feel free to suggest optional items as well.
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> 1) API
> > > > > >>>>>>>>>>>>> 1.1) API compatibility *MUST* be maintained between
> > > > > >>> minor
> > > > > >>>>>>>> releases.
> > > > > >>>>>>>>>> Do
> > > > > >>>>>>>>>>>> not
> > > > > >>>>>>>>>>>>> remove existing methods or change their signatures,
> > > > > >>>>> deprecate
> > > > > >>>>>>>> them
> > > > > >>>>>>>>>>>> instead
> > > > > >>>>>>>>>>>>> 1.2) Default behaviour "SHOULD NOT* be changed
> between
> > > > > >>>>> minor
> > > > > >>>>>>>>>> releases,
> > > > > >>>>>>>>>>>>> unless absolutely needed. If change is made, it
> *MUST*
> > > > > >>> be
> > > > > >>>>>>>> described
> > > > > >>>>>>>>>> in
> > > > > >>>>>>>>>>>>> "Migration Guide"
> > > > > >>>>>>>>>>>>> 1.3) New operation *MUST* be well-documented in code
> > > > > >>>>>> (javadoc,
> > > > > >>>>>>>>>>>> dotnetdoc):
> > > > > >>>>>>>>>>>>> documentation must contain method's purpose,
> > > > > >> description
> > > > > >>>>> of
> > > > > >>>>>>>>>> parameters
> > > > > >>>>>>>>>>>> and
> > > > > >>>>>>>>>>>>> how their values affect the outcome, description of
> > > > > >>> return
> > > > > >>>>>>> value
> > > > > >>>>>>>>> and
> > > > > >>>>>>>>>>> it's
> > > > > >>>>>>>>>>>>> default, behavior in negative cases, interaction with
> > > > > >>>>> other
> > > > > >>>>>>>>>> operations
> > > > > >>>>>>>>>>>> and
> > > > > >>>>>>>>>>>>> components
> > > > > >>>>>>>>>>>>> 1.4) API parity between Java and .NET platforms
> > > > > >> *SHOULD*
> > > > > >>>>> be
> > > > > >>>>>>>>>> maintained
> > > > > >>>>>>>>>>>> when
> > > > > >>>>>>>>>>>>> operation makes sense on both platforms. If method
> > > > > >>> cannot
> > > > > >>>>> be
> > > > > >>>>>>>>>>> implemented
> > > > > >>>>>>>>>>>> in
> > > > > >>>>>>>>>>>>> a platform immediately, new JIRA ticket *MUST* be
> > > > > >>> created
> > > > > >>>>> and
> > > > > >>>>>>>>> linked
> > > > > >>>>>>>>>> to
> > > > > >>>>>>>>>>>>> current ticket
> > > > > >>>>>>>>>>>>> 1.5) API parity between thin clients (Java, .NET)
> > > > > >>>>> *SHOULD* be
> > > > > >>>>>>>>>>> maintained
> > > > > >>>>>>>>>>>>> when operation makes sense on several clients. If
> > > > > >> method
> > > > > >>>>>> cannot
> > > > > >>>>>>>> be
> > > > > >>>>>>>>>>>>> implemented in a client immediately, new JIRA ticket
> > > > > >>>>> *MUST*
> > > > > >>>>>> be
> > > > > >>>>>>>>>> created
> > > > > >>>>>>>>>>>> and
> > > > > >>>>>>>>>>>>> linked to current ticket
> > > > > >>>>>>>>>>>>> 1.6) All exceptions thrown to a user *MUST* have
> > > > > >>>>> explanation
> > > > > >>>>>>> how
> > > > > >>>>>>>> to
> > > > > >>>>>>>>>>>>> resolve, workaround or debug an error
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> 2) Compatibility
> > > > > >>>>>>>>>>>>> 2.1) Persistence backward compatibility *MUST* be
> > > > > >>>>> maintained
> > > > > >>>>>>>>> between
> > > > > >>>>>>>>>>>> minor
> > > > > >>>>>>>>>>>>> releases. It should be possible to start newer
> version
> > > > > >>> on
> > > > > >>>>>> data
> > > > > >>>>>>>>> files
> > > > > >>>>>>>>>>>>> created by the previous version
> > > > > >>>>>>>>>>>>> 2.2) Thin client forward and backward compatibility
> > > > > >>>>> *SHOULD*
> > > > > >>>>>> be
> > > > > >>>>>>>>>>>> maintained
> > > > > >>>>>>>>>>>>> between two consecutive minor releases. If
> > > > > >> compatibility
> > > > > >>>>>> cannot
> > > > > >>>>>>>> be
> > > > > >>>>>>>>>>>>> maintained it *MUST* be described in "Migration
> Guide"
> > > > > >>>>>>>>>>>>> 2.3) JDBC and ODBC forward and backward compatibility
> > > > > >>>>>> *SHOULD*
> > > > > >>>>>>> be
> > > > > >>>>>>>>>>>>> maintained between two consecutive minor releases. If
> > > > > >>>>>>>> compatibility
> > > > > >>>>>>>>>>>> cannot
> > > > > >>>>>>>>>>>>> be maintained it *MUST* be described in "Migration
> > > > > >>> Guide"
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> 3) Tests
> > > > > >>>>>>>>>>>>> 3.1) New functionality *MUST* be covered with unit
> > > > > >> tests
> > > > > >>>>> for
> > > > > >>>>>>> both
> > > > > >>>>>>>>>>>> positive
> > > > > >>>>>>>>>>>>> and negative use cases
> > > > > >>>>>>>>>>>>> 3.2) All test suites *MUST* be run before merge to
> > > > > >>>>>>> master..There
> > > > > >>>>>>>>>> *MUST*
> > > > > >>>>>>>>>>>> be
> > > > > >>>>>>>>>>>>> no new test failures
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> 4) Code style *MUST* be followed as per Ignite's
> > > > > >> Coding
> > > > > >>>>>>>> Guidelines
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> Vladimir.
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> [1] https://www.ietf.org/rfc/rfc2119.txt
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>> On Fri, May 4, 2018 at 4:33 PM, Vladimir Ozerov <
> > > > > >>>>>>>>>> vozerov@gridgain.com>
> > > > > >>>>>>>>>>>>> wrote:
> > > > > >>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>> Hi Dmitry,
> > > > > >>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>> Yes, I'll do that in the nearest days.
> > > > > >>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>> On Wed, Apr 25, 2018 at 8:24 PM, Dmitry Pavlov <
> > > > > >>>>>>>>>>> dpavlov.spb@gmail.com>
> > > > > >>>>>>>>>>>>>> wrote:
> > > > > >>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>> Igniters, the idea was related to small
> > > > > >> refactorings
> > > > > >>>>>>>> co-located
> > > > > >>>>>>>>>> with
> > > > > >>>>>>>>>>>>> main
> > > > > >>>>>>>>>>>>>>> change.
> > > > > >>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>> Main change itself indicates that existing code did
> > > > > >>> not
> > > > > >>>>>> meet
> > > > > >>>>>>>> the
> > > > > >>>>>>>>>>>>> criteria
> > > > > >>>>>>>>>>>>>>> of practice. Approving of standalone refactorings
> > > > > >>>>> instead
> > > > > >>>>>>>>>>> contradicts
> > > > > >>>>>>>>>>>>> with
> > > > > >>>>>>>>>>>>>>> principle don't touch if it works. So I still like
> > > > > >>>>> idea of
> > > > > >>>>>>>>>>> co-located
> > > > > >>>>>>>>>>>>>>> changes improving code, javadocs, style, etc.
> > > > > >>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>> But let's not argue about this point now, let's
> > > > > >>>>> summarize
> > > > > >>>>>>> the
> > > > > >>>>>>>>>>>> undisputed
> > > > > >>>>>>>>>>>>>>> points and add it to the wiki. Vladimir, would you
> > > > > >>>>> please
> > > > > >>>>>> do
> > > > > >>>>>>>> it?
> > > > > >>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>> ср, 25 апр. 2018 г. в 16:42, Nikolay Izhikov <
> > > > > >>>>>>>>> nizhikov@apache.org
> > > > > >>>>>>>>>>> :
> > > > > >>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>> Igniters,
> > > > > >>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>> I agree with Vova.
> > > > > >>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>> Don't fix if it works!
> > > > > >>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>> If you 100% sure then it a useful addition to the
> > > > > >>>>>> product
> > > > > >>>>>>> -
> > > > > >>>>>>>>> just
> > > > > >>>>>>>>>>>> make
> > > > > >>>>>>>>>>>>> a
> > > > > >>>>>>>>>>>>>>>> separate ticket.
> > > > > >>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>> В Ср, 25/04/2018 в 11:44 +0300, Vladimir Ozerov
> > > > > >>>>> пишет:
> > > > > >>>>>>>>>>>>>>>>> Guys,
> > > > > >>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>> The problem with in-place refactorings is that
> > > > > >>> you
> > > > > >>>>>>>> increase
> > > > > >>>>>>>>>>>> affected
> > > > > >>>>>>>>>>>>>>>> scope.
> > > > > >>>>>>>>>>>>>>>>> It is not uncommon to break compatibility or
> > > > > >>> public
> > > > > >>>>>>>>> contracts
> > > > > >>>>>>>>>>> with
> > > > > >>>>>>>>>>>>>>> even
> > > > > >>>>>>>>>>>>>>>>> minor things. E.g. recently we decided drop
> > > > > >>>>> org.jsr166
> > > > > >>>>>>>>> package
> > > > > >>>>>>>>>>> in
> > > > > >>>>>>>>>>>>>>> favor
> > > > > >>>>>>>>>>>>>>>> of
> > > > > >>>>>>>>>>>>>>>>> Java 8 classes. Innocent change. Result -
> > > > > >> broken
> > > > > >>>>>>> storage.
> > > > > >>>>>>>>>>> Another
> > > > > >>>>>>>>>>>>>>> problem
> > > > > >>>>>>>>>>>>>>>>> is conflicts. It is not uncommon to have
> > > > > >>> long-lived
> > > > > >>>>>>>> branches
> > > > > >>>>>>>>>>> which
> > > > > >>>>>>>>>>>>> we
> > > > > >>>>>>>>>>>>>>>> need
> > > > > >>>>>>>>>>>>>>>>> to merge with master over and over again. And a
> > > > > >>>>> lot of
> > > > > >>>>>>>>>>>> refactorings
> > > > > >>>>>>>>>>>>>>> cause
> > > > > >>>>>>>>>>>>>>>>> conflicts. It is much easier to resolve them if
> > > > > >>> you
> > > > > >>>>>> know
> > > > > >>>>>>>>> that
> > > > > >>>>>>>>>>>> logic
> > > > > >>>>>>>>>>>>>>> was
> > > > > >>>>>>>>>>>>>>>> not
> > > > > >>>>>>>>>>>>>>>>> affected as opposed to cases when you need to
> > > > > >>>>> resolve
> > > > > >>>>>>> both
> > > > > >>>>>>>>>>> renames
> > > > > >>>>>>>>>>>>> and
> > > > > >>>>>>>>>>>>>>>>> method extractions along with business-logic
> > > > > >>>>> changes.
> > > > > >>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>> I'd like to repeat - if you have a time for
> > > > > >>>>>> refactoring
> > > > > >>>>>>>> then
> > > > > >>>>>>>>>> you
> > > > > >>>>>>>>>>>>>>>> definitely
> > > > > >>>>>>>>>>>>>>>>> have a time to extract these changes to
> > > > > >> separate
> > > > > >>> PR
> > > > > >>>>>> and
> > > > > >>>>>>>>>> submit a
> > > > > >>>>>>>>>>>>>>> separate
> > > > > >>>>>>>>>>>>>>>>> ticket. I am quite understand what "low
> > > > > >> priority"
> > > > > >>>>> do
> > > > > >>>>>> you
> > > > > >>>>>>>>> mean
> > > > > >>>>>>>>>> if
> > > > > >>>>>>>>>>>> you
> > > > > >>>>>>>>>>>>>>> do
> > > > > >>>>>>>>>>>>>>>>> refactorings on your own.
> > > > > >>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>> On Tue, Apr 24, 2018 at 10:52 PM, Andrey
> > > > > >>> Kuznetsov
> > > > > >>>>> <
> > > > > >>>>>>>>>>>>> stkuzma@gmail.com
> > > > > >>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>> wrote:
> > > > > >>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> +1.
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> Once again, I beg for "small refactoring
> > > > > >>>>> permission"
> > > > > >>>>>>> in
> > > > > >>>>>>>> a
> > > > > >>>>>>>>>>>>> checklist.
> > > > > >>>>>>>>>>>>>>>> As of
> > > > > >>>>>>>>>>>>>>>>>> today, separate tickets for small
> > > > > >> refactorings
> > > > > >>>>> has
> > > > > >>>>>>>> lowest
> > > > > >>>>>>>>>>>>> priority,
> > > > > >>>>>>>>>>>>>>>> since
> > > > > >>>>>>>>>>>>>>>>>> they neither fix any flaw nor add new
> > > > > >>>>> functionality.
> > > > > >>>>>>>> Also,
> > > > > >>>>>>>>>> the
> > > > > >>>>>>>>>>>>>>>> attempts to
> > > > > >>>>>>>>>>>>>>>>>> make issue-related code safer / cleaner /
> > > > > >> more
> > > > > >>>>>>> readable
> > > > > >>>>>>>> in
> > > > > >>>>>>>>>>>> "real"
> > > > > >>>>>>>>>>>>>>> pull
> > > > > >>>>>>>>>>>>>>>>>> requests are typically rejected, since they
> > > > > >>>>>> contradict
> > > > > >>>>>>>> our
> > > > > >>>>>>>>>>>> current
> > > > > >>>>>>>>>>>>>>>>>> guidelines.
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> I understand this will require a bit more
> > > > > >>> effort
> > > > > >>>>>> from
> > > > > >>>>>>>>>>>>>>>> committer/maintainer,
> > > > > >>>>>>>>>>>>>>>>>> but otherwise we will get constantly
> > > > > >> degrading
> > > > > >>>>> code
> > > > > >>>>>>>>> quality.
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> 2018-04-24 18:52 GMT+03:00 Eduard Shangareev
> > > > > >> <
> > > > > >>>>>>>>>>>>>>>> eduard.shangareev@gmail.com
> > > > > >>>>>>>>>>>>>>>>>>> :
> > > > > >>>>>>>>>>>>>>>>>>> Vladimir,
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> I am not talking about
> > > > > >> massive/sophisticated
> > > > > >>>>>>>>> refactoring.
> > > > > >>>>>>>>>>> But
> > > > > >>>>>>>>>>>> I
> > > > > >>>>>>>>>>>>>>>> believe
> > > > > >>>>>>>>>>>>>>>>>>> that ask to extract some methods should be
> > > > > >> OK
> > > > > >>>>> to
> > > > > >>>>>> do
> > > > > >>>>>>>>>> without
> > > > > >>>>>>>>>>> an
> > > > > >>>>>>>>>>>>>>> extra
> > > > > >>>>>>>>>>>>>>>>>>> ticket.
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> A checklist shouldn't be necessarily a set
> > > > > >> of
> > > > > >>>>>>> certain
> > > > > >>>>>>>>>> rules
> > > > > >>>>>>>>>>>> but
> > > > > >>>>>>>>>>>>>>> also
> > > > > >>>>>>>>>>>>>>>> it
> > > > > >>>>>>>>>>>>>>>>>>> could include suggestion and reminders.
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> On Tue, Apr 24, 2018 at 6:39 PM, Vladimir
> > > > > >>>>> Ozerov <
> > > > > >>>>>>>>>>>>>>>> vozerov@gridgain.com>
> > > > > >>>>>>>>>>>>>>>>>>> wrote:
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> Ed,
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> Refactoring is a separate task. If you
> > > > > >>> would
> > > > > >>>>>> like
> > > > > >>>>>>> to
> > > > > >>>>>>>>>>> rework
> > > > > >>>>>>>>>>>>>>>> exchange
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> future
> > > > > >>>>>>>>>>>>>>>>>>>> - please do this in a ticket "Refactor
> > > > > >>>>> exchange
> > > > > >>>>>>>> task",
> > > > > >>>>>>>>>>>> nobody
> > > > > >>>>>>>>>>>>>>> would
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> against
> > > > > >>>>>>>>>>>>>>>>>>>> this. This is just a matter of creating
> > > > > >>>>> separate
> > > > > >>>>>>>>> ticket
> > > > > >>>>>>>>>>> and
> > > > > >>>>>>>>>>>>>>>> separate
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> PR.
> > > > > >>>>>>>>>>>>>>>>>>> If
> > > > > >>>>>>>>>>>>>>>>>>>> one have a time for refactoring, it
> > > > > >> should
> > > > > >>>>> not
> > > > > >>>>>> be
> > > > > >>>>>>> a
> > > > > >>>>>>>>>>> problem
> > > > > >>>>>>>>>>>>> for
> > > > > >>>>>>>>>>>>>>>> him to
> > > > > >>>>>>>>>>>>>>>>>>>> spend several minutes on JIRA and GitHub.
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> As far as documentation - what you
> > > > > >> describe
> > > > > >>>>> is
> > > > > >>>>>>>> normal
> > > > > >>>>>>>>>>> review
> > > > > >>>>>>>>>>>>>>>> process,
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> when
> > > > > >>>>>>>>>>>>>>>>>>>> reviewer might want to ask contributor to
> > > > > >>> fix
> > > > > >>>>>>>>> something.
> > > > > >>>>>>>>>>>>>>> Checklist
> > > > > >>>>>>>>>>>>>>>> is a
> > > > > >>>>>>>>>>>>>>>>>>>> different thing - this is a set of rules
> > > > > >>>>> which
> > > > > >>>>>>> must
> > > > > >>>>>>>> be
> > > > > >>>>>>>>>>>>> followed
> > > > > >>>>>>>>>>>>>>> by
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> anyone.
> > > > > >>>>>>>>>>>>>>>>>>>> I do not understand how you can define
> > > > > >>>>>>> documentation
> > > > > >>>>>>>>> in
> > > > > >>>>>>>>>>> this
> > > > > >>>>>>>>>>>>>>>> checklist.
> > > > > >>>>>>>>>>>>>>>>>>>> Same problem with logging - what is
> > > > > >>> "enough"?
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> On Tue, Apr 24, 2018 at 4:51 PM, Eduard
> > > > > >>>>>>> Shangareev <
> > > > > >>>>>>>>>>>>>>>>>>>> eduard.shangareev@gmail.com> wrote:
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>> Igniters,
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>> I don't understand why you are so
> > > > > >> against
> > > > > >>>>>>>>> refactoring.
> > > > > >>>>>>>>>>>>>>>>>>>>> Code already smells like hell. Methods
> > > > > >>> 200+
> > > > > >>>>>> line
> > > > > >>>>>>>> is
> > > > > >>>>>>>>>>>> normal.
> > > > > >>>>>>>>>>>>>>>> Exchange
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> future
> > > > > >>>>>>>>>>>>>>>>>>>>> is asking to be separated on several
> > > > > >> one.
> > > > > >>>>>>>>> Transaction
> > > > > >>>>>>>>>>> code
> > > > > >>>>>>>>>>>>>>> could
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> understand
> > > > > >>>>>>>>>>>>>>>>>>>>> few people.
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>> If we separate refactoring from
> > > > > >>>>> development it
> > > > > >>>>>>>> would
> > > > > >>>>>>>>>>> mean
> > > > > >>>>>>>>>>>>> that
> > > > > >>>>>>>>>>>>>>>> no one
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> will
> > > > > >>>>>>>>>>>>>>>>>>>>> do it.
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>> 2) Documentation.
> > > > > >>>>>>>>>>>>>>>>>>>>> Everything which was asked by reviewers
> > > > > >>> to
> > > > > >>>>>>> clarify
> > > > > >>>>>>>>>> idea
> > > > > >>>>>>>>>>>>>>> should be
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> reflected
> > > > > >>>>>>>>>>>>>>>>>>>>> in the code.
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>> 3) Logging.
> > > > > >>>>>>>>>>>>>>>>>>>>> Logging should be enough to
> > > > > >> troubleshoot
> > > > > >>>>> the
> > > > > >>>>>>>> problem
> > > > > >>>>>>>>>> if
> > > > > >>>>>>>>>>>>>>> someone
> > > > > >>>>>>>>>>>>>>>> comes
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> to
> > > > > >>>>>>>>>>>>>>>>>>>>> user-list with an issue in the code.
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>> On Fri, Apr 20, 2018 at 7:06 PM, Dmitry
> > > > > >>>>>> Pavlov <
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> dpavlov.spb@gmail.com>
> > > > > >>>>>>>>>>>>>>>>>>>>> wrote:
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> Hi Igniters,
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> +1 to idea of checklist.
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> +1 to refactoring and documenting
> > > > > >> code
> > > > > >>>>>> related
> > > > > >>>>>>>> to
> > > > > >>>>>>>>>>> ticket
> > > > > >>>>>>>>>>>>> in
> > > > > >>>>>>>>>>>>>>>> +/-20
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> LOC
> > > > > >>>>>>>>>>>>>>>>>>>> at
> > > > > >>>>>>>>>>>>>>>>>>>>>> least.
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> If we start to do it as part of our
> > > > > >>>>> regular
> > > > > >>>>>>>>>>>> contribution,
> > > > > >>>>>>>>>>>>>>> code
> > > > > >>>>>>>>>>>>>>>> will
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> be
> > > > > >>>>>>>>>>>>>>>>>>>>>> better, it would became common
> > > > > >> practice
> > > > > >>>>> and
> > > > > >>>>>>> part
> > > > > >>>>>>>>> of
> > > > > >>>>>>>>>>>> Apache
> > > > > >>>>>>>>>>>>>>>> Ignite
> > > > > >>>>>>>>>>>>>>>>>>>>>> development culure.
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> If we will hope we will have free
> > > > > >> time
> > > > > >>> to
> > > > > >>>>>>> submit
> > > > > >>>>>>>>>>>> separate
> > > > > >>>>>>>>>>>>>>> patch
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> someday
> > > > > >>>>>>>>>>>>>>>>>>>>> and
> > > > > >>>>>>>>>>>>>>>>>>>>>> have patience to complete
> > > > > >>>>> patch-submission
> > > > > >>>>>>>>> process,
> > > > > >>>>>>>>>>> code
> > > > > >>>>>>>>>>>>>>> will
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> remain
> > > > > >>>>>>>>>>>>>>>>>>>>>> undocumented and poor-readable.
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> Sincerely,
> > > > > >>>>>>>>>>>>>>>>>>>>>> Dmitriy Pavlov
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> пт, 20 апр. 2018 г. в 18:56,
> > > > > >> Александр
> > > > > >>>>>>>> Меньшиков <
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> sharplermc@gmail.com
> > > > > >>>>>>>>>>>>>>>>>>>>> :
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>> 4) Metrics.
> > > > > >>>>>>>>>>>>>>>>>>>>>>> partially +1
> > > > > >>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>> It makes sense to have some minimal
> > > > > >>>>> code
> > > > > >>>>>>>>> coverage
> > > > > >>>>>>>>>>> for
> > > > > >>>>>>>>>>>>> new
> > > > > >>>>>>>>>>>>>>>> code in
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> PR.
> > > > > >>>>>>>>>>>>>>>>>>>>>> IMHO.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>> Also, we can limit the cyclomatic
> > > > > >>>>>> complexity
> > > > > >>>>>>>> of
> > > > > >>>>>>>>>> the
> > > > > >>>>>>>>>>>> new
> > > > > >>>>>>>>>>>>>>> code
> > > > > >>>>>>>>>>>>>>>> in
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> PR
> > > > > >>>>>>>>>>>>>>>>>>>> too.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>> 6) Refactoring
> > > > > >>>>>>>>>>>>>>>>>>>>>>> -1
> > > > > >>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>> I understand why people want to
> > > > > >>>>> refactor
> > > > > >>>>>> old
> > > > > >>>>>>>>> code.
> > > > > >>>>>>>>>>>>>>>>>>>>>>> But I think refactoring should be
> > > > > >>>>> always a
> > > > > >>>>>>>>>> separate
> > > > > >>>>>>>>>>>>> task.
> > > > > >>>>>>>>>>>>>>>>>>>>>>> And it's better to remove all
> > > > > >>>>> refactoring
> > > > > >>>>>>> from
> > > > > >>>>>>>>> PR,
> > > > > >>>>>>>>>>> if
> > > > > >>>>>>>>>>>>> it's
> > > > > >>>>>>>>>>>>>>>> not
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> the
> > > > > >>>>>>>>>>>>>>>>>>>>> sense
> > > > > >>>>>>>>>>>>>>>>>>>>>> of
> > > > > >>>>>>>>>>>>>>>>>>>>>>> the issue.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>> 2018-04-20 16:54 GMT+03:00 Andrey
> > > > > >>>>>> Kuznetsov
> > > > > >>>>>>> <
> > > > > >>>>>>>>>>>>>>>> stkuzma@gmail.com>:
> > > > > >>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> What about adding the following
> > > > > >>> item
> > > > > >>>>> to
> > > > > >>>>>>> the
> > > > > >>>>>>>>>>>> checklist:
> > > > > >>>>>>>>>>>>>>>> when the
> > > > > >>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>> change
> > > > > >>>>>>>>>>>>>>>>>>>>>>> adds
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> new functionality, then unit
> > > > > >> tests
> > > > > >>>>>> should
> > > > > >>>>>>>> also
> > > > > >>>>>>>>>> be
> > > > > >>>>>>>>>>>>>>>> provided, if
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> it's
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> technically possible?
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> As for refactorings, in fact they
> > > > > >>> are
> > > > > >>>>>>>> strongly
> > > > > >>>>>>>>>>>>>>> discouraged
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> today
> > > > > >>>>>>>>>>>>>>>>>>>> for
> > > > > >>>>>>>>>>>>>>>>>>>>>> some
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> unclear reason. Let's permit to
> > > > > >>> make
> > > > > >>>>>>>>>> refactorings
> > > > > >>>>>>>>>>> in
> > > > > >>>>>>>>>>>>> the
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> checklist
> > > > > >>>>>>>>>>>>>>>>>>>>>> being
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> discussed. (Of cource,
> > > > > >> refactoring
> > > > > >>>>>> should
> > > > > >>>>>>>>> relate
> > > > > >>>>>>>>>>> to
> > > > > >>>>>>>>>>>>>>> problem
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> being
> > > > > >>>>>>>>>>>>>>>>>>>>>>> solved.)
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> 2018-04-20 16:16 GMT+03:00
> > > > > >> Vladimir
> > > > > >>>>>>> Ozerov <
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> vozerov@gridgain.com
> > > > > >>>>>>>>>>>>>>>>>>>> :
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> Hi Ed,
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> Unfortunately some of these
> > > > > >>> points
> > > > > >>>>> are
> > > > > >>>>>>> not
> > > > > >>>>>>>>>> good
> > > > > >>>>>>>>>>>>>>>> candidates
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> for
> > > > > >>>>>>>>>>>>>>>>>>>> the
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> checklist because of these:
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> - It must be clear and disallow
> > > > > >>>>>>> *multiple
> > > > > >>>>>>>>>>>>>>>> interpretations*
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> - It must be *lightweight*,
> > > > > >>>>> otherwise
> > > > > >>>>>>>> Ignite
> > > > > >>>>>>>>>>>>>>> development
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> would
> > > > > >>>>>>>>>>>>>>>>>>>>>> become a
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> nightmare
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> We cannot have "nice to have"
> > > > > >>>>> points
> > > > > >>>>>>> here.
> > > > > >>>>>>>>>>>> Checklist
> > > > > >>>>>>>>>>>>>>>> should
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> answer
> > > > > >>>>>>>>>>>>>>>>>>>>>> the
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> question "is ticket eligible to
> > > > > >>> be
> > > > > >>>>>>>> merged?"
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> 1) Code style.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> +1
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> 2) Documentation
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> -1, it is impossible to define
> > > > > >>>>> what is
> > > > > >>>>>>>>>>>>>>>> "well-documented". A
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> piece
> > > > > >>>>>>>>>>>>>>>>>>>>> of
> > > > > >>>>>>>>>>>>>>>>>>>>>>> code
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> could be obvious for one
> > > > > >>>>> contributor,
> > > > > >>>>>>> and
> > > > > >>>>>>>>>>>>> non-obvious
> > > > > >>>>>>>>>>>>>>> for
> > > > > >>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>> another.
> > > > > >>>>>>>>>>>>>>>>>>>>> In
> > > > > >>>>>>>>>>>>>>>>>>>>>>> any
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> case this is not a blocker for
> > > > > >>>>> merge.
> > > > > >>>>>>>>> Instead,
> > > > > >>>>>>>>>>>>> during
> > > > > >>>>>>>>>>>>>>>> review
> > > > > >>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>> one
> > > > > >>>>>>>>>>>>>>>>>>>>> can
> > > > > >>>>>>>>>>>>>>>>>>>>>>> ask
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> implementer to add more docs,
> > > > > >> but
> > > > > >>>>> it
> > > > > >>>>>>>> cannot
> > > > > >>>>>>>>> be
> > > > > >>>>>>>>>>>>> forced.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> 3) Logging
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> -1, same problem - what is
> > > > > >>> "enough
> > > > > >>>>>>>>> logging?".
> > > > > >>>>>>>>>>>> Enough
> > > > > >>>>>>>>>>>>>>> for
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> whom?
> > > > > >>>>>>>>>>>>>>>>>>>> How
> > > > > >>>>>>>>>>>>>>>>>>>>> to
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> understand whether it is enough
> > > > > >>> or
> > > > > >>>>>> not?
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> 4) Metrics
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> -1, no clear boundaries, and
> > > > > >>>>> decision
> > > > > >>>>>> on
> > > > > >>>>>>>>>> whether
> > > > > >>>>>>>>>>>>>>> metrics
> > > > > >>>>>>>>>>>>>>>> are
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> to
> > > > > >>>>>>>>>>>>>>>>>>>> be
> > > > > >>>>>>>>>>>>>>>>>>>>>>> added
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> or
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> not should be performed during
> > > > > >>>>> design
> > > > > >>>>>>>> phase.
> > > > > >>>>>>>>>> As
> > > > > >>>>>>>>>>>>>>> before,
> > > > > >>>>>>>>>>>>>>>> it is
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> perfectly
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> valid to ask contributor to add
> > > > > >>>>>> metrics
> > > > > >>>>>>>> with
> > > > > >>>>>>>>>>> clear
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> explanation
> > > > > >>>>>>>>>>>>>>>>>>>> why,
> > > > > >>>>>>>>>>>>>>>>>>>>>> but
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> this is not part of the
> > > > > >>> checklist.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> 5) TC status
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> +1, already mentioned
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>>>> 6) Refactoring
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> Strong -1. OOP is a slippery
> > > > > >>> slope,
> > > > > >>>>>>> there
> > > > > >>>>>>>>> are
> > > > > >>>>>>>>>> no
> > > > > >>>>>>>>>>>>> good
> > > > > >>>>>>>>>>>>>>>> and bad
> > > > > >>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>> receipts
> > > > > >>>>>>>>>>>>>>>>>>>>>>>> for
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> all cases, hence it cannot be
> > > > > >>> used
> > > > > >>>>> in
> > > > > >>>>>> a
> > > > > >>>>>>>>>>> checklist.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> We can borrow useful rules from
> > > > > >>>>> p.2,
> > > > > >>>>>> p.3
> > > > > >>>>>>>> and
> > > > > >>>>>>>>>> p.4
> > > > > >>>>>>>>>>>> if
> > > > > >>>>>>>>>>>>>>> you
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> provide
> > > > > >>>>>>>>>>>>>>>>>>>>> clear
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> definitions on how to measure
> > > > > >>> them.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> Vladimir.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> On Fri, Apr 20, 2018 at 3:50
> > > > > >> PM,
> > > > > >>>>>> Eduard
> > > > > >>>>>>>>>>>> Shangareev <
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>> eduard.shangareev@gmail.com>
> > > > > >>>>> wrote:
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>> Also, I want to add some
> > > > > >>>>> technical
> > > > > >>>>>>>>>>> requirement.
> > > > > >>>>>>>>>>>>>>> Let's
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> discuss
> > > > > >>>>>>>>>>>>>>>>>>>>> them.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>> 1) Code style.
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>>> The code needs to be
> > > > > >> formatted
> > > > > >>>>>>> according
> > > > > >>>>>>>>> to
> > > > > >>>>>>>>>>>> coding
> > > > > >>>>>>>>>>>>>>>>>>
> > > > > >>>>>>>>>>>>>>>>>> guidelines
> > > > > >>>>>>>>>>>>>>>>>>>>>>>>

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