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 > > > > >
