<div dir="ltr"><div>Matt,<br><br></div><div>I appreciate that it has barely been 2 months since we've started following the new model of collaboration (see thread subject :), so the intent of highlighting Bogdan's work was exactly what you said: put it on your radar.<br><br></div><div>We have discussed the swift ring rebalance patch in the IRC meeting yesterday:<br><a href="http://eavesdrop.openstack.org/meetings/puppet_openstack/2015/puppet_openstack.2015-08-18-15.00.log.html#l-108">http://eavesdrop.openstack.org/meetings/puppet_openstack/2015/puppet_openstack.2015-08-18-15.00.log.html#l-108</a><br></div><div><div><div><br></div><div>You've now confirmed what Emilien said in the meeting: core reviewers in puppet-swift don't know enough about ring rebalance to judge this patch on their own, and need an expert opinion from Swift team. That's totally fine in my book and I don't have any issue with that.<br><br></div><div>I also don't have an issue with the -1 that's still outstanding: it's a valid concern and it was good to raise it. I think Alex has addressed it in his rebuttal, and now it needs two more votes to move forward: a comment from a Puppet expert to confirm or disprove Alex's statement that his code is idemptotent and will not spuriously trigger rebalance when it's not needed, and one more from Christian to confirm that the idempotency assurance is sufficient to address his concern.<br></div><div><br></div><div>My problem was that it took a month and two escalations on weekly IRC meeting and another one here before this situation was explained. A better way to handle this would have been to leave a +1 (or even 0) with a comment along the lines of "Puppet code looks good, but we need a Swift expert to confirm this, Christian please have a look."<br><br></div><div>In other words, it never hurts to over-communicate :)<br><br></div><div>Thanks,<br></div><div>-DmitryB<br></div><div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 19, 2015 at 11:47 AM Matt Fischer <<a href="mailto:matt@mattfischer.com">matt@mattfischer.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Dmitry,<div><br></div><div>I've appreciated the feedback on my patches from your team and the work they are doing, it's great that everyone is working together better now. I think getting more puppet core reviewers is certainly on the horizon and will happen with continued effort, it just takes time and trust. But its "on the radar" to use a analogy.</div><div><br></div><div>As for your specific issue with the swift patch, I don't know enough about ring builder to decide whether the author or the swift reviewer is right so I was hoping he (the swift core) would follow-up to your comment. The puppet code itself is fine. I've also asked someone on my team who is a swift expert (but not a puppet core) to take a look and weigh-in. I don't know what our official policy is, but I would expect your author to reach out to the person who left the -1 and attempt to resolve it with them before one of us would essentially override the -1.<br><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 18, 2015 at 12:33 AM, Dmitry Borodaenko <span dir="ltr"><<a href="mailto:dborodaenko@mirantis.com" target="_blank">dborodaenko@mirantis.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Two weeks ago, I flagged the patch sets to commits ratio as the biggest<br>
problem that Fuel contributors to Puppet OpenStack should address, and<br>
over the past two weeks the situation has improved dramatically. In<br>
first and second week of August, 19 of our commits were merged in<br>
upstream, bringing our patch sets per commit ratio from 19 down to 5.6<br>
(while average for Puppet OpenStack during that period was 6.5). With<br>
the share of patch sets pushed by Fuel developers remaining at roughly<br>
the same level (15.9% vs 17.4%), I think it's safe to call this problem<br>
solved. Simply awesome!<br>
<br>
Comparing last 30 days contribution stats vs same numbers two weeks ago:<br>
<br>
Bogdan Dobrelia (#3 reviewer!): 67.2% -> 66.3% (disagreements 4.9% -> 3.6%)<br>
Denis Egorenko: 97% -> 87.5% (disagreements 12.1% -> 13.9%)<br>
Vasyl Saienko: 100% -> 96.4% (disagreements 16.7% -> 10.7%)<br>
Ivan Berezovskiy: 100% -> 92.3% (disagreements 0% -> 3.8%)<br>
Sergey Kolekonov: 91.7% -> 95.7% (disagreements 8.3% -> 13%)<br>
Max Yatsenko: n/a -> 100% (disagreements n/a -> 17.4%)<br>
Alex Schultz: 80% -> 80% (disagreements 20% -> 26.7%)<br>
Bartlomiej Piotrowski: n/a -> 100% (disagreements n/a -> 12.5%)<br>
Sergii Golovatiuk: 100% -> 100% (disagreements 33.3% -> 0%)<br>
<br>
Bogdan continues to set the example and improve his numbers, which is<br>
not surprising considering that he's also the top reviewer in<br>
fuel-library. I think puppet-nova and puppet-neutron teams should<br>
seriously consider nominating him for core, he already tops reviewers<br>
lists for these modules.<br>
<br>
Denis, Vasyl, and Ivan are not there yet, but they all have noticeably<br>
increased both number and quality of their reviews, keep it up guys!<br>
<br>
Numbers for other top reviewers are uneven and small enough for noise to<br>
overtake meaningful data, all I can recommend here is to watch your<br>
disagreements and learn from them. A ratio above 10% can mean one of<br>
three things: 1) you're not doing enough reviews so even a handful of<br>
disagreements sticks out -- do more reviews and this will improve on its<br>
own; 2) you're missing problems with the code that other reviewers find<br>
unacceptable -- try to be more attentive and watch for the things that<br>
you've been missing; 3) you disagree with majority on how some things<br>
should be done -- discuss your differences on IRC or ML and figure out a<br>
consensus.<br>
<br>
Moving on to other numbers, weekly IRC meetings participation remains<br>
good:<br>
<br>
Aug-4: 4 of 15 participants, 22 of 162 lines<br>
Aug-11: 6 of 16 participants, 62 of 192 lines<br>
<br>
Unfortunately it's not all roses and unicorns, last week I flagged a<br>
stuck review that I think has been mishandled by Puppet OpenStack team<br>
[0][1], and it still remains in the same state.<br>
<br>
[0] <a href="https://review.openstack.org/198695" rel="noreferrer" target="_blank">https://review.openstack.org/198695</a><br>
[1] <a href="http://eavesdrop.openstack.org/meetings/puppet_openstack/2015/puppet_openstack.2015-08-11-15.00.log.html#l-140" rel="noreferrer" target="_blank">http://eavesdrop.openstack.org/meetings/puppet_openstack/2015/puppet_openstack.2015-08-11-15.00.log.html#l-140</a><br>
<br>
On July 8, patch set 2 has passed CI. It received 2 +1 votes from other<br>
Fuel developers on July 10 and 29, but remained ignored by upstream<br>
reviewers until August 10, more than a month after the current patch set<br>
was posted. Then, a Swift core reviewer left a -1 disagreeing with the<br>
intent of the patch, and even though patch author posted a rebuttal a<br>
day later, the patch remains stuck and untouched for yet another week.<br>
<br>
It's a one-off case that does not outweigh the positive trends I've<br>
outlined above, but even one stuck patch that fixes a critical bug is<br>
enough to justify a fork.<br>
<br>
Speaking of forks, we managed to un-fork 9 upstream modules [2] with<br>
puppet-librarian-simple before Fuel 7.0 soft code freeze has kicked in.<br>
<br>
[2] <a href="https://github.com/stackforge/fuel-library/blob/master/deployment/Puppetfile" rel="noreferrer" target="_blank">https://github.com/stackforge/fuel-library/blob/master/deployment/Puppetfile</a><br>
<br>
That's 9 times more than my conservative estimate of converting at least<br>
1 module to librarian in 7.0, but these were the easiest and least<br>
deviated from upstream. We've still got 50 more modules to convert in<br>
Fuel 8.0, many will require commits to be merged in upstream before they<br>
can be completely un-forked. Having such commits wait for a month at a<br>
time only to be summarily rejected puts this effort at risk, lets figure<br>
out what went wrong this time and come up with a way to prevent this<br>
from happening again.<br>
<br>
Thanks,<span><font color="#888888"><br>
<br>
-- <br>
Dmitry Borodaenko<br>
<br>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</font></span></blockquote></div><br></div>
__________________________________________________________________________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.openstack.org?subject:unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</a><br>
</blockquote></div></div></div></div></div>