Re: Code reviews in Beam

I do think we need something less generic than our current @Experimental. I also like the idea of a separate package for unvetted contributions (though Incubating might simply be confusing, given that Apache uses Incubating for something else). Good idea to have a standard way of marking such

Re: Code reviews in Beam

Thanks, Reuven, for bringing this up. This is an area well worth investing time in. Specific comments below. On Mon, Feb 19, 2018 at 10:32 AM, Reuven Lax wrote: > Pedantic > > Overly-pedantic comments (change variable names, etc.) can be frustrating. > The PR author can feel

Re: Code reviews in Beam

I'm ambivalent about the placement of less-stable contributions, but I'd be very much in favor of being clear about how stable a given API is. There's several axes of stability here, too: - Is the API going to stay compatible - Is the implementation going to stay compatible via pipeline updates -

Re: Code reviews in Beam

On further thought I like the separate package even more. It allows us to easily isolate all those tests, and not block commits or releases on them. On Tue, Feb 20, 2018 at 7:45 AM, Reuven Lax wrote: > Another idea: we could have a special package for these "unrefined" >

Re: Code reviews in Beam

I would add some @ToBeRefined or so  Le 20 févr. 2018 à 16:35, à 16:35, Reuven Lax 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

Re: Code reviews in Beam

Fully agree ! Thanks Reuven Regards JB Le 20 févr. 2018 à 16:35, à 16:35, Reuven Lax 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

Re: Code reviews in Beam

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

Re: Code reviews in Beam

+1. It's a fair idea to have dedicated guides. Regards JB Le 20 févr. 2018 à 14:43, à 14:43, Alexey Romanenko a écrit: >Reuven, thank you for bringing this topic. > >As a new contributor to Beam codebase I raise two my hands for such >guideline document and I'd

Re: Code reviews in Beam

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

Re: Code reviews in Beam

Reuven, thank you for bringing this topic. As a new contributor to Beam codebase I raise two my hands for such guideline document and I'd propose to add it as a new guide into section “Other Guides” on web site documentation. For sure, there are already several very helpful and detailed

Re: Code reviews in Beam

This is excellent! I can't really add anything right now but I think having a PR dashboard is one of the most important points because it also indirectly solves "Review Latency" and "Code Review Response SLA" by making things more visible. -- Aljoscha > On 19. Feb 2018, at 19:32, Reuven Lax