Yea, that's a nice idea Romain. I would support trying to move code to master behind a flag ASAP.
The thing to remember is this: if/when a feature branch moves to master it is too large to review all together. So the reviews to get things onto a feature branch need to be the same quality and clarity standard as master. If it is a "hack" branch then the code needs to be re-reviewed, so the branch itself should not plan on merging directly but instead have the code copied into new PRs for full review. Kenn On Thu, Apr 12, 2018 at 9:36 PM Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > Maybe add a toggle in flinkoptions to activate it until it is tested and > you are happy with it? Kind of --flinkExperimental=sdf,log,... (random > values). This allows to hit master and continue to work on it. > > Le 13 avr. 2018 03:07, "Robert Bradshaw" <rober...@google.com> a écrit : > > Thomas captures exactly my concerns. > > I think we should look at getting everything we can into master (at least > filing JIRAs, the work may take longer) and move what development we can > there. What remains would be a collection of hacks (mostly in the SDKs, but > it's not a feature branch 'cause we'd never want to actually merge it in) > that hopefully we can whittle away (again, JIRAs should be filed for the > items there). > On Thu, Apr 12, 2018 at 5:44 PM Henning Rohde <hero...@google.com> wrote: > > > + 1 to capture in JIRA what needs to be done. > > > The simplest path forward might be to reimplement/cherrypick'n'modify the > changes onto master directly. We would then effectively just abandon the > hacking branch and treat code there as a prototype. Although we would add > components without end2end verification initially, it would allow parallel > progress across the SDKs and the Flink runner. The Go SDK is also already > in master and can help test the migration of the Flink runner changes > before the other SDKs are ready. > > > On Thu, Apr 12, 2018 at 4:44 PM Thomas Weise <t...@apache.org> wrote: > > >> Strong +1 on transitioning all development to the ASF repo. > > >> I think a straight move of the hacking branch may still be problematic > though, because it sets the path to continue hacking vs. working towards a > viable milestone that other contributors can base their work off. I would > prefer a state that separates serious development from hacks in a way where > the code does not overlap. > > >> Based on Ben's assessment, if most of the hacks are currently are in the > SDK area, then perhaps we can transition everything related to job server > and translation to master so that it is possible to build and work on the > runner there and then only use the hacked SDKs branch for demos? > > >> And maybe discuss an MVP milestone and put together a JIRA view that > shows what remains to be done to get there? > > >> Thanks, > >> Thomas > > > >> On Thu, Apr 12, 2018 at 4:26 PM, Holden Karau <hol...@pigscanfly.ca> > wrote: > > >>> So I would be strongly in favour of adding it as a branch on the Apache > repo. This way other folks are more likely to be able to help with the > splitting up and merging process and also while Flink forward is behind us > getting in the practice of doing feature branches on the ASF repo for > collaboration instead of personal github accounts seems like a worthy goal. > > >>> On Thu, Apr 12, 2018 at 4:21 PM Robert Bradshaw <rober...@google.com> > wrote: > > >>>> I suppose with the hackathon and flink forward behind us, I'm thinking > we > >>>> should start shifting gears more getting what we have into master in > >>>> production state and less on continuing working on a hacking branch. > If we > >>>> think it'll fairly quick there's no big need to create an official > branch, > >>>> and if it's going to be long lived perhaps we should rethink our > process. > >>>> On Thu, Apr 12, 2018 at 3:44 PM Aljoscha Krettek <aljos...@apache.org > > > >>>> wrote: > > >>>> > I would also be in favour of adding a branch to our main repo. A > random > >>>> branch on some personal GitHub account can seem a bit sketchy and > adding a > >>>> branch to our repo could make it more visible for people that are > >>>> interested. > > > > >>>> > On 12. Apr 2018, at 15:29, Ben Sidhom <sid...@google.com> wrote: > > >>>> > I would say that most of it is not suitable for direct merging. > There are > >>>> several reasons for this: > > >>>> > Most changes are built on upstream PRs that are either not submitted > or > >>>> have been rebased before submission. > >>>> > There are some very hacky changes in the Python and Java SDKs to get > >>>> portable pipelines working. For example, hard coding certain options > and/or > >>>> baking dependencies into the SDK harness images. These need to be > actually > >>>> implemented correctly in their respective SDKs. > >>>> > Much of the code does not have proper tests and fails simple lint > tests. > > >>>> > As a concrete example, I tried cherry-picking the changes from > >>>> https://github.com/bsidhom/beam/pull/46 into master. This is a > relatively > >>>> simple change, but there were so many merge conflicts that in the end > it > >>>> was easier to just reimplement the changes atop master. More > importantly, > >>>> most changes will require refactoring before actually going in. > > >>>> > On Thu, Apr 12, 2018 at 3:16 PM, Robert Bradshaw < > rober...@google.com > > >>>> wrote: > > >>>> >> How much of this is not suitable to merging into master directly > (not as > >>>> >> is, but as separate PRs)? > >>>> >> On Thu, Apr 12, 2018 at 3:10 PM Ben Sidhom <sid...@google.com> > wrote: > > >>>> >> > Hey all, > > >>>> >> > I've been working on a proof-of-concept portable Flink runner > with some > >>>> >> other Beam contributors. We would like to have a point of reference > for > >>>> the > >>>> >> rest of the Beam community as we integrate this work into master. > It > >>>> >> currently lives under > >>>> >> https://github.com/bsidhom/beam/tree/hacking-job-server. > > >>>> >> > I would suggest pulling this into the main ASF repo under an > >>>> >> appropriately-named branch (flink-portable-hacking?). The name > should > >>>> >> suggest the intention that this branch is not intended to be pulled > into > >>>> >> master as-is and that it should rather be used as a reference for > now. > > >>>> >> > Thoughts? > > >>>> >> > -- > >>>> >> > -Ben > > > > > >>>> > -- > >>>> > -Ben > > >>> -- > >>> Twitter: https://twitter.com/holdenkarau > > >
