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, 24 Apr 2018 19:52:23 GMT
+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.

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