Another idea: we could have a special package for these "unrefined" contributions. Once the contribution has had time to mature some, it can be moved to the regular package structure.
On Tue, Feb 20, 2018 at 7:41 AM, Jean-Baptiste Onofré <j...@nanthrax.net> wrote: > I would add some @ToBeRefined or so 😁 > Le 20 févr. 2018, à 16:35, Reuven Lax <re...@google.com> a écrit: >> >> Hi JB, >> >> You're right, I was thinking more about changes to core when talking >> about the technical-excellence bar. >> >> I think there still needs to be some bar for new features and extension, >> but I also think it can be much lower (as nobody is breaking anything by >> merging this). An example of where we still need a bar here is tests. If a >> new IO has a test that the reviewer thinks will be flaky, that flaky test >> will cause problems for _every_ Beam committer, and it's fair to ask for >> the test to be changed. >> >> Given that the bar is lower for new extensions, I think we need a good >> way of marking these things so that Beam users know they are not as mature >> as other parts of Beam. Traditionally we've used @Experimental, but >> @Experimental has been overloaded to mean other things as well. Maybe we >> need to introduce a new annotation? >> >> Reuven >> >> On Tue, Feb 20, 2018 at 5:48 AM, Jean-Baptiste Onofré <j...@nanthrax.net> >> wrote: >> >>> Hi Reuven >>> >>> I agree with all your points except maybe in term of bar level, >>> especially on new features (like extensions or IOs). If the PRs on the core >>> should be heavily reviewed, I'm more in favor of merging the PR pretty fast >>> even if not perfect. It's not a technical topic, it's really a contribution >>> and community topic. >>> >>> Thanks anyway, the dashboard is a good idea ! >>> >>> Regards >>> JB >>> Le 19 févr. 2018, à 19:33, Reuven Lax < re...@google.com> a écrit: >>>> >>>> There have been a number of threads on code reviews (most recently on a >>>> "State of the project" email). These threads have died out without much >>>> resolution, but I'm not sure that the concerns have gone away. >>>> >>>> First of all, I'm of the opinion that a code-review bar for Beam >>>> commits is critical to success of the project. This is a system with many >>>> subtle semantics, which might not be obvious at first glance. Beam >>>> pipelines process user data, and the consequence of certain bugs might mean >>>> corrupting user data and aggregations - something to avoid at all cost if >>>> we want Beam to be trusted. Finally Beam pipelines often run at extremely >>>> high scale; while many of our committers have a strong intuition for what >>>> can go wrong when running at high scale, not everybody who wants to >>>> contribute will have this experience. >>>> >>>> >>>> However, we also cannot afford to let our policy get in the way of >>>> building a community. We *must* remain a friendly place to develop and >>>> contribute. >>>> >>>> >>>> When I look at concerns people have had on on code reviews (and I've >>>> been browsing most PRs this past year), I see a few common threads: >>>> >>>> >>>> *Review Latency* >>>> >>>> Latency on code reviews can be too high. At various times folks (most >>>> recently, Ahmet and I) have tried to regularly look for stale PRs and ping >>>> them, but latency still remains high. >>>> >>>> >>>> *Pedantic* >>>> >>>> Overly-pedantic comments (change variable names, etc.) can be >>>> frustrating. The PR author can feel like they are being forced to make >>>> meaningless changes just so the reviewer will allow merging. Note that this >>>> is sometimes in the eye of the beholder - the reviewer may not think all >>>> these comments are pedantic. >>>> >>>> >>>> *Don't Do This* >>>> >>>> Sometimes a reviewer rejects an entire PR, saying that this should not >>>> be done. There are various reasons given: this won't scale, this will break >>>> backwards compatibility, this will break a specific runner, etc. The PR >>>> author may not always understand or agree with these reasons, and this can >>>> leave hurt feelings. >>>> >>>> >>>> I would like open discussion about ways of making our code-review >>>> policy more welcoming. I'll seed the discussion with a few ideas: >>>> >>>> >>>> *Code Review Dashboard and Automation* >>>> >>>> We should invest in adding a code-review dashboard to our site, >>>> tracking stale PRs by reviewer. Quick turnaround on code reviews is >>>> essential building community, so all Beam committers should consider >>>> reviewing code as important as their own coding. Spark has built a PR >>>> dashboard (https://spark-prs.appspot.com/) which they’ve found better >>>> than Github’s dashboard; we could easily fork this dashboard. There are >>>> also tools that will automatically ping reviewers (mention-bot and hey >>>> there are two such tools). We can also make sure that new PRs are auto >>>> assigned a reviewer (e.g. https://github.com/imsky/pull-review) >>>> >>>> >>>> *Code Review Response SLA* >>>> >>>> It would be great if we could agree on a response-time SLA for Beam >>>> code reviews. The response might be “I am unable to do the review until >>>> next week,” however even that is better than getting no response. >>>> >>>> >>>> *Guideline Document* >>>> >>>> I think we should have a guideline document, explaining common reasons >>>> a reviewer might reject an approach in a PR. e.g. "This will cause scaling >>>> problems," "This will cause problems for XXX runner," "This is backwards >>>> incompatible." Reviewers can point to this doc as part of their comments, >>>> along with extra flavor. e.g. “as per the guideline doc, this will cause >>>> scaling problems, and here’s why.” >>>> >>>> >>>> *Guidelines on Comments* >>>> >>>> Not everyone agrees on which comments are pedantic, which makes it hard >>>> to have specific guidelines here. One general guideline me might adopt: if >>>> it'll take less time for the reviewer to make the changes themselves, it's >>>> not an appropriate comment. The reviewer should fix those issues a >>>> follow-on PR. This adds a bit more burden on reviewers, but IMO is worth it >>>> to keep Beam a friendly environment. This is especially important for >>>> first-time contributors, who might otherwise lost interest. If the author >>>> is a seasoned Beam contributor, we can expect more out of them. >>>> >>>> >>>> We should also make sure that these fixups serve as educational moments >>>> for the new contributor. “Thanks for the contribution! I’ll be making some >>>> changes during the merge so that the code stays consistent across the >>>> codebase - keep an eye on them.” >>>> >>>> >>>> Would love to hear more thoughts. >>>> >>>> >>>> Reuven >>>> >>>> >>
