ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vladimir Ozerov <voze...@gridgain.com>
Subject Re: Ticket review checklist
Date Mon, 04 Jun 2018 08:31:42 GMT
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
> > >>>>>>>>>>>>>>>>>>>>>>>>>> <
> > >>>>>>>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>>>>>>> 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/
> > >>>>>>>>>>>>>>>>>>
> > >>>>>>>>>>>>>>>>>> conf
>

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