I am for RTC. Agree with Val, that significant part of the concurrency issues are very hard to catch with CI. They are caught on review (and some unfortunately slip through). And Sergi's points seems very valid to me - complex changes should be reviewed to make the best effort to avoid issues of any type.
I would encourage everyone to request review if: - core logic changed. Although it is very hard to define, but communication, discovery, cache, compute, deployment, continuous processes are on the list. - public APIs - new feature completed (in most cases it causes changes to public API) The following can be committed to master right-away: - Javadocs - Comments - Typos - Fixes to tests Let's summarize the points to some kind of guideline/hints and put it on wiki. --Yakov 2015-07-28 11:18 GMT+03:00 Valentin Kulichenko < valentin.kuliche...@gmail.com>: > I tend to agree on points brought by Brane and Cos. Having a formality that > review is always required regardless of what the change actually implies > looks like an overkill. > At the same time, I usually feel uncomfortable without a review. Ignite is > not just a "complicated" project. It has a lot of concurrency issues (both > local and distributed) and a lot of components doing absolutely different > stuff, but highly integrated with each other. As a result, there are tons > of race conditions which CI will never check and it's really possible to > miss something. > I would follow (and would recommend to others) the RTC model in most cases, > but I don't need a formal rule for this. > > -Val > On Jul 28, 2015 12:57 AM, "Dmitriy Setrakyan" <dsetrak...@apache.org> > wrote: > > > I want to address the following point made by Brane and Cos: > > ---- > > > > *if you don't trust a committer to make the changes in the VCSsystem - > that > > committer shouldn't be having the commit-bit in the first place.* > > ---- > > > > I don't believe this point actually applies to Ignite. > > > > As all of us are aware, Ignite has a very lightweight process for a > > contributor to become a committer. Essentially, once any type of > > contribution is accepted, even a 1-liner, a contributor is eligible to > > become a committer. > > > > Such process was put in place to ensure that community can grow faster, > and > > allows new members to quickly start contributing in a more meaningful > way. > > Consequently, the same policy will result in some not-so-experienced > > committers joining the project. > > > > I personally like the fact that we have a very easy path for the > new-comers > > to join. However, for as long as we have this policy, I believe that RTC > > remains a better approach for us. > > > > Having said that, I would agree that certain trivial fixes can be made > > directly in the master branch. For example, simple documentation or test > > updates, IMHO, would fit into this category, as long as an email is sent > to > > the dev list notifying the rest of the community about the change. > > > > D. > > > > On Mon, Jul 27, 2015 at 4:30 PM, Konstantin Boudnik <c...@apache.org> > > wrote: > > > > > On Tue, Jul 28, 2015 at 01:09AM, Branko Čibej wrote: > > > > On 27.07.2015 18:29, Sergi Vladykin wrote: > > > > > Guys, > > > > > > > > > > I would say Ignite is quite a big and quite complex project. > > > > > > > > This has absolutely nothing to do with trusting developers. FWIW, > > > > Subversion is also very complex, I'll dare say that its object model > > and > > > > working copy semantics are more convoluted than anything you'll find > in > > > > Ignite. We make do with CTR quite well. > > > > > > > > > > > > > Also we have > > > > > really tough requirements for performance, stability, backward > > > > > compatibility, etc... > > > > > > > > Write them down. Make sure new contributors understand them before > > > > offering them commit access. That's a prerequisite for growing an > open > > > > community anyway. > > > > > > > > > Having said that it is really easy to break something > > > > > even with minor change like switching type of collection to another > > > one. > > > > > > > > So? Stuff like that should be documented in the code, if it isn't no > > > > amount of prior review will help, since how do you know that the > > > > reviewer happens to remember such details a few years from now? > > > > > > > > > And I personally feel safer when my code has been reviewed before I > > > merge > > > > > it to master. > > > > > > > > So ask for a review if you think you need it. I never said *all* > > reviews > > > > should be post-commit: I said that the default mode should be CTR and > > > > it's up to the developer to ask for more eyes on their code. > > > > > > > > > Also when thousand lines change set is merged and there are > > > > > conflicting change sets merged after, it is quite hard to rollback > > this > > > > > first change if it was wrong. And we have conflicting changes all > the > > > time. > > > > > > > > Apparently you need to learn to use the version control tool you > > > > selected, or adjust your workflows for the failings of said tool. Or > > > > change the tool, who knows, you might find that's a good move. :) > > > > > > > > Regardless of which tool you use, conflicts should *ALWAYS* be > resolved > > > > on the development branch, not on the mainline: the workflow should > be: > > > > > > > > 1. create branch from mainline > > > > 2. make changes on branch > > > > 3. merge mainline to branch and resolve conflicts (repeat as needed) > > > > 4. run tests on branch code > > > > 5. merge branch to mainline > > > > > > > > Skipping step 3 (and 4) is going to be a constant pain regardless of > > > > which tool you use. Also for trivial changes it makes no sense at all > > to > > > > even create a branch; just fix it on the mainline, your version > history > > > > will be a lot easier to follow. > > > > > > > > > My opinion is that correct trade off for us now is having slower > but > > > more > > > > > predictable development. RTC approach here definitely fits better. > > > > > > > > RTC has an additional problem that it can (and often does; look at > one > > > > of the most famous ASF projects if you don't believe me) create > > tensions > > > > in the community. It makes sense to use it for maintenance branches, > > but > > > > not for new development. > > > > > > Needless to say, I agree with everything just said (except for the VCS > > > change > > > proposition, which I found to be outright crazy, of course ;) > > > > > > And to pile more on top: I've experienced the consequences of the very > > > situation, described in the very last paragraph. The RTC lead to a > > totally > > > nuts situation in Hadoop, where fellas who wanted to reject a small > > patch, > > > not > > > based on the technical merits of the fix, had came up with an excuse > that > > > the > > > reviewer (a full committer) didn't have enough expertise to review the > > > code in > > > question. The silliness was quickly put down by the sane community > > members, > > > but you see where I am going with it... > > > > > > In other words: if you don't trust a committer to make the changes in > the > > > VCS > > > system - that committer shouldn't be having the commit-bit in the first > > > place. > > > Intricacies of roll-backs aren't an excuse and most often than not > could > > be > > > fixed by changing the process hygiene. CTR attitude leads, in general, > > to a > > > higher quality of the contribution because ppl are trying to avoid > public > > > scolding, inevitable in case of forced-reverts. With RTC the blame > would > > be > > > shared between the author and the reviewer, which make situation more > > > bearable > > > (falsely, of course). And guess what - reviewers are humans and make > > > mistakes > > > all the time, but that was already pointed out. > > > > > > Cos > > > > > > > > >
