<div dir="ltr"><div><div><div><div>The patch @ <a href="http://review.gluster.org/#/c/15716/1">http://review.gluster.org/#/c/15716/1</a> introduces whatever was asked for - a test script that enables compound fops and exercises the affected codepath. Note that the script also caught some bugs in the code which<br></div>have been fixed as part of the same patch.<br><br></div>@Niels - Request you to revoke the -2s on the 4 existing compound fops patches and have it merged once the<br></div>final patch (the one that introduces the script) passes regression.<br><br></div>-Krutika<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 15, 2016 at 2:40 PM, Niels de Vos <span dir="ltr"><<a href="mailto:ndevos@redhat.com" target="_blank">ndevos@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Sep 07, 2016 at 10:42:15PM +0530, Pranith Kumar Karampuri wrote:<br>
> On Wed, Sep 7, 2016 at 10:21 PM, Pranith Kumar Karampuri <<br>
> <a href="mailto:pkarampu@redhat.com">pkarampu@redhat.com</a>> wrote:<br>
><br>
> ><br>
> ><br>
> > On Wed, Sep 7, 2016 at 8:00 PM, Niels de Vos <<a href="mailto:ndevos@redhat.com">ndevos@redhat.com</a>> wrote:<br>
> ><br>
> >> Hi Krutika and Pranith,<br>
> >><br>
> >> The patches that introduce the compound operations for release-3.8 are<br>
> >> almost ready for merging. Is there a particular order for the merging to<br>
> >> be done? I'd hate to break git-bisect if I do it in the wrong order.<br>
> >><br>
> >> <a href="http://review.gluster.org/#/q/status:open+project:glusterfs+" rel="noreferrer" target="_blank">http://review.gluster.org/#/q/<wbr>status:open+project:glusterfs+</a><br>
> >> branch:release-3.8+topic:bug-<wbr>1372693<br>
> >><br>
> >> These massive changes +1500 lines of code, have not one test. Eventhough<br>
> >> a new volume option is introduced. I really do hope there is some test<br>
> >> in the master branch that runs soemthing useful with the new option<br>
> >> enabled. Could you please backport that test-case too?<br>
> >><br>
> ><br>
> > These patches are stabilization of experimental feature compound-fops for<br>
> > 3.8.x that is why you see massive changes because it is still stabilizing.<br>
> > As per the guidelines it is okay to do this. As this is performance<br>
> > enhancement, the nature of tests we did were manual and comparing the time<br>
> > elapsed with and without the fixes. We see 10% performance improvement<br>
> > overall in small file create workload where the write workload is 40% where<br>
> > the enhancement comes into picture. That is the reason you don't see any<br>
> > automated test as I don't think we can reliably test the performance<br>
> > improvement at the moment in automation. May be we can integrate it with<br>
> > the one Nigel & Shyam are driving once it is available.<br>
> ><br>
> ><br>
> >><br>
> >> Because this adds a new functionality based on the (experimental)<br>
> >> compount FOPs, we need to mention it in the release notes. Feel free to<br>
> >> start a new doc/release-notes/<a href="http://3.8.x.md" rel="noreferrer" target="_blank">3.8.x.md</a> with a little text about the<br>
> >> goal of feature, how users can enable it and verify results.<br>
> >><br>
> ><br>
> > Hmm... it is not a feature, it is stabilization of feature and using it.<br>
> > We can probably add a short description of how the option needs to be used<br>
> > to take advantage of the perf enhancement in the bz. It can be taken as<br>
> > part of the normal process of aggregating release notes.<br>
> ><br>
> ><br>
> >><br>
> >> I really want us to prevent introducing such a lot of new code, that<br>
> >> does not go through regular testing. Should this be included in 3.8.4,<br>
> >> or maybe the release after it?<br>
> >><br>
> ><br>
> > For it to go through regular testing, by default the option<br>
> > use-compound-fops needs to be enabled, but since all of this is still<br>
> > experimental, I would like it to be disabled. What are the tentative<br>
> > release dates for 3.8.4/3.8.5?<br>
> ><br>
><br>
> I see that you plan to release it sometime next week. I think it is safer<br>
> to merge these just after the tagging so that it will go to 3.8.5. It will<br>
> also lessen the probability of surprises.<br>
<br>
</div></div>Yes, these patches are candidates for 3.8.5. However, I *REALLY* demand<br>
some minimal automated testing. At the very least, enable the option and<br>
do a few operations that use the COMPOUND procedures.<br>
<br>
Thanks,<br>
Niels<br>
</blockquote></div><br></div>