On Wed, Feb 20, 2013 at 2:49 PM, Suresh Srinivas <sur...@hortonworks.com> wrote: > Todd, > > Some of us have been trying to help test and review the code. However you > might have missed the following, which has resulted in the review not > completing: > > 02/06/13 - After intent for merge was sent, I posted comment saying > consolidate patch has extraneous changes. That was non trivial amount of > extraneous changes.
This was just an error with the "consolidate merge patch". Like I said in the previous email, these patches are just for Jenkins QA to run on, and I assume that any HDFS committer is able to look at the branch itself to understand the changes in it. It's easy to accidentally end up with extraneous changes when you try to generate these merge patches - eg the same thing happened to you earlier this week on HADOOP-8562 if I'm not mistaken. > 02/06/13 - Nicholas posted some comments and also indicated previous > unaddressed comments. Colin addressed this feedback on 2/8 in https://issues.apache.org/jira/browse/HDFS-4476 . Nicholas chose not to review the changes (though acknowledged the JIRA on 2/6), so Aaron committed it a week later. > 02/15/13 - No update was made to consolidated patch. I stopped reviewing it > waiting for the new patch. A new patch gets posted on 2/15 and soon after > merge vote email on 2/17/13 during the long weekend. > Again, all the changes are in the branch. Again I can't imagine trying to review a merge of a branch by looking at a 400KB patch. They're just there to trigger Jenkins. It should be the responsibility of committers to look at the branch itself. Or if you prefer a single patch, it's trivial to generate one in your local repo. > At this time, some of the comments that were made earlier have not been > addressed. Also folks who were reviewing the consolidated patch have not > posted +1. It seems like the best way to trigger people actually reviewing branches is to call merge votes. I would have hoped that people would review the work as it went along. If we waited for a +1 without calling a merge vote, this would drag on for months and months. This is based on my experience with the 3 or 4 branches I've worked on. > > I think we should wait for +1 for the merge patch (from the folks actively > reviewing the patch) before the merge vote. That might make this process > smoother. But I agree, if the changes are deemed to be trivial, we can do > it post merge to trunk. The difficulty is defining "actively reviewing the patch". Making 3 or 4 cursory comments once every 2 weeks doesn't look like "active review" to me. On the other hand, I spent probably 40-50% of my time over the last month reviewing and testing this branch and have voted +1. -Todd > > > On Wed, Feb 20, 2013 at 12:16 PM, Todd Lipcon <t...@cloudera.com> wrote: > >> Hi Nicholas, >> >> I looked at your comments on the JIRA, and they all seem like trivial >> things that could be addressed post-merge, and none of them would >> affect the functionality. If Colin addresses these issues, will you >> amend your vote to +1 within the called-for voting period? >> >> It concerns me that we've been asking for reviews on this branch for >> multiple months now, and yet you're only bringing up some of these >> things now that a merge vote is called. Colin sentp a note to this >> list a month ago (http://markmail.org/message/phcfc3watwlqiemw) saying >> that the merge was coming soon. Since then, we found a few small bugs >> around the configuration/setup code, but all of the things you're >> bringing up in the review now have been in the branch since the new >> year, so I feel like there has been quite ample time for review. >> >> -Todd >> >> On Wed, Feb 20, 2013 at 11:56 AM, Tsz Wo Sze <szets...@yahoo.com> wrote: >> > -1 >> > The patch seems not ready yet. I have posted some comments/suggestions >> on the JIRA. Colin also has agreed that there are some bugs to be fixed. >> Sorry. >> > >> > Tsz-Wo >> > >> > >> > >> > >> > ________________________________ >> > From: Todd Lipcon <t...@cloudera.com> >> > To: hdfs-dev@hadoop.apache.org >> > Sent: Tuesday, February 19, 2013 4:11 PM >> > Subject: Re: VOTE: HDFS-347 merge >> > >> > +1 (binding) >> > >> > I code-reviewed almost all of the code in this branch, and also spent >> some >> > time benchmarking and testing under various workloads. We've also done >> > significant testing on clusters here at Cloudera, both secure and >> insecure, >> > and verified integration with a number of other ecosystem components (eg >> > Pig, Hive, Impala, HBase, MR, etc). The feature works as advertised and >> > should provide much better performance for a number of workloads, >> > especially in secure environments. >> > >> > Thanks for the hard work, Colin! >> > >> > -Todd >> > >> > On Sun, Feb 17, 2013 at 1:48 PM, Colin McCabe <cmcc...@alumni.cmu.edu >> >wrote: >> > >> >> Hi all, >> >> >> >> I would like to merge the HDFS-347 branch back to trunk. It's been >> >> under intensive review and testing for several months. The branch >> >> adds a lot of new unit tests, and passes Jenkins as of 2/15 [1] >> >> >> >> We have tested HDFS-347 with both random and sequential workloads. The >> >> short-circuit case is substantially faster [2], and overall >> >> performance looks very good. This is especially encouraging given >> >> that the initial goal of this work was to make security compatible >> >> with short-circuit local reads, rather than to optimize the >> >> short-circuit code path. We've also stress-tested HDFS-347 on a >> >> number of clusters. >> >> >> >> This iniial VOTE is to merge only into trunk. Just as we have done >> >> with our other recent merges, we will consider merging into branch-2 >> >> after the code has been in trunk for few weeks. >> >> >> >> Please cast your vote by EOD Sunday 2/24. >> >> >> >> best, >> >> Colin McCabe >> >> >> >> [1] >> >> >> https://issues.apache.org/jira/browse/HDFS-347?focusedCommentId=13579704&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13579704 >> >> >> >> [2] >> >> >> https://issues.apache.org/jira/browse/HDFS-347?focusedCommentId=13551755&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13551755 >> >> >> > >> > >> > >> > -- >> > Todd Lipcon >> > Software Engineer, Cloudera >> >> >> >> -- >> Todd Lipcon >> Software Engineer, Cloudera >> > > > > -- > http://hortonworks.com/download/ -- Todd Lipcon Software Engineer, Cloudera
