reef-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dongjoon Hyun <dongj...@apache.org>
Subject Re: "The overhead of reviewing and testing"
Date Wed, 27 Jan 2016 18:23:34 GMT
Thank you, Gon and Andrew.

I agree with your opinions and suggestions.

Especially, for the '[MINOR]' tag, '[MINOR]' would be great if that reduce
the burden of reviewers really. Is it used before really? So far, I cannot
find any commit having '[MINOR]' in its title.

If we have a documentation about HOW TO REVIEW the MINOR tags, that would
be really helpful for us. And, if possible, any guideline about what is
minor tag? I'm not sure about this part. Usually, the MINOR tag is the
intention of contributor, not it of reviewer.

I hope it reduces the number of JIRA issues and maybe some testing if we
trust our CI.
For example, the following 1~4 are really small minor implementation
changes. Can we skip manually testing step for these during reviews if they
have explicit '[MINOR]' tag? Of course, one of our three CI should passes.
The purpose of running three REEF CI was 'Speculative Testing
<http://mail-archives.apache.org/mod_mbox/reef-dev/201510.mbox/%3CCAFu6yH3kiX%2BYpHPVGETXoN91MYMfckQviqM7a8%2BHzQRCNROdLA%40mail.gmail.com%3E>
'.

1. [REEF-1150] Use `static` members properly by removing access by
`instance` reference
2. [REEF-1151] Remove illformed `String.format` usage in
TestClassHierarchyRoundTrip
3. [REEF-1152] Remove duplicate plugin declaration in reef-vortex.
4. [REEF-1153] Fix typos in class/function names




On Wed, Jan 27, 2016 at 10:13 AM, Andrew Chung <afchung90@gmail.com> wrote:

> Thanks for your comment Dongjoon!
>
> I can understand your argument in that:
>
>    1. REEF would like to have minor tasks that don't require extensive
>    knowledge of the codebase such as fixing typos such that we can allow
> newer
>    contributors to get the gratification of contributing to the codebase
> and
>    hopefully induce them to making more contributions in the future and
>    2. blocking a PR based on work outside of the scope of the PR itself or
>    posing hard questions to a contributor (automating typo detection, in
> this
>    case) may make the new contributors feel unwelcome.
>
> I can also understand Mariia's argument as to how certain simple processes
> should be automated.
>
> Personally, I am fine with reviewing and merging smaller PRs, but
> automation is ultimately something that would make everyone's life easier.
> Ideally, I would prefer harder discussions to be moved to the mailing list
> and/or opening separate JIRA items for it and move the discussion there
> instead of posing questions directly in the same PR as such to avoid making
> the contribution feel unwelcome, even though it may not be the intention of
> the reviewer -- after all, as contributors to the open source project, we
> are ultimately all trying to improve the codebase quality. :)
>
> What does everyone else think?
>
> Thanks,
> Andrew
>
> On Wed, Jan 27, 2016 at 1:49 AM, Byung-Gon Chun <bgchun@gmail.com> wrote:
>
> > Hi,
> >
> > We had related discussion in August 2015.
> >
> >
> https://mail-archives.apache.org/mod_mbox/incubator-reef-dev/201508.mbox/%3C55DB2CDE.40804@weimo.de%3E
> >
> > The [MINOR] tag has worked really well for SNU internal projects. I
> > strongly recommend the reef community adopts it. :-)
> >
> > Thanks!
> > -Gon
> >
> >
> >
> > On Wed, Jan 27, 2016 at 5:11 PM, Dongjoon Hyun <dongjoon@apache.org>
> > wrote:
> >
> > > Oh, I missed your word, "We've had this discussion from the other side
> > > before".
> > > It's not a different story. Sorry!
> > >
> > > It's too late night. I had better go to bed before making another
> > mistake.
> > >
> > > Dongjoon.
> > >
> > >
> > > On Tue, Jan 26, 2016 at 11:51 PM, Dongjoon Hyun <dongjoon@apache.org>
> > > wrote:
> > >
> > > > Thank you for your generous comments. Actually, what I meant to focus
> > is
> > > > the vice versa.
> > > >
> > > > "The reviewer (in a review cycle) should focus on only the PR code
> > itself
> > > > like Blind Paper Review Process."
> > > >
> > > > - Why does a reviewer consider a contributor's other PRs?
> > > > - Why does a reviewer ask something beyond the scope of the PR?
> > > >   (As we know, contributors are not REEF's or PMC's full-time
> > employees.)
> > > >
> > > > I dream that an ideal environment where any tiny contributions from
> > > anyone
> > > > are welcome equally(here, blindly) anytime.
> > > >
> > > > Although the content of PR does not improve REEF, a reviewer should
> > say a
> > > > warm comment like 'Sorry, but thanks for making a PR', and give -1. I
> > > think
> > > > that's natural in Open Source Communities. (The PR code will be
> > > withdrawed
> > > > or closed.)
> > > >
> > > > I know that REEF community consists of strong developers and has warm
> > > > atmosphere in general. I wrote here because I was surprise about the
> > > > concept of `overhead`. I really wanted to listen others' opinions,
> too.
> > > > Thank you, Markus.
> > > >
> > > > Dongjoon.
> > > >
> > > >
> > > >
> > > > On Tue, Jan 26, 2016 at 10:58 PM, Markus Weimer <markus@weimo.de>
> > wrote:
> > > >
> > > >> Hi,
> > > >>
> > > >> thanks for bringing this up. It is an interesting, different angle
> on
> > > the
> > > >> review process. We've had this discussion from the other side
> before:
> > > >> contributors felt that the review cycle put a impediment onto them.
> > > >>
> > > >> I believe both arguments have a kernel of validity, but I also
> believe
> > > >> that reviews are super-crucial not only for code quality, but even
> > more
> > > so
> > > >> for a shared understanding of the code. As you said, every committer
> > and
> > > >> PMC member should use the opportunity to stay current with REEF's
> > > >> development via the review process. To be honest, that is often my
> > > reason
> > > >> for picking specific PRs: I want to know what's happening in that
> part
> > > of
> > > >> the code.
> > > >>
> > > >> That said, I think we should use every tool and idea available to
> > remove
> > > >> friction from the process. Basic coding standard checks and test
> runs
> > > >> shouldn't be the work of human brains, but CPUs :) The work Mariia
> and
> > > you
> > > >> have been doing goes a long way towards that. There are some gaping
> > > holes
> > > >> (*ahem* .NET tests) which should be addressed soon.
> > > >>
> > > >> But I am rambling: What do other's think? Is the PR review process
> too
> > > >> burdensome? What (beyond the basic continuous integration setup) can
> > we
> > > do
> > > >> to make it easier?
> > > >>
> > > >> Thanks again for bringing this up!
> > > >>
> > > >> Markus
> > > >>
> > > >
> > > >
> > >
> >
> >
> >
> > --
> > Byung-Gon Chun
> >
>

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