[webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
Hi, Please don't roll out patches speculatively unless that's the only way to diagnose the problem. Even then you should really go talk to authors and make sure they're okay with it. And please re-land patches that didn't cause test failures or regressions promptly once you've fixed or diagnosed the issue. It's extremely rude to roll out someone else's patch speculatively and then leave. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
I'll have to disagree with you here. If the build is broken and the gardener/build cop has a strong reason to suspect that it was caused by a specific patch and the author is unavailable then rolling that patch out is the right thing to do. It might inconvenience the author but it is the responsibility of the author and reviewer to make sure the patch didn't break anything. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 12:17 PM, Emil A Eklund e...@chromium.org wrote: I'll have to disagree with you here. If the build is broken and the gardener/build cop has a strong reason to suspect that it was caused by a specific patch and the author is unavailable then rolling that patch out is the right thing to do. It author is unavailable is the key statement here. That said, if your strong reason turned out to be incorrect, you should recommit the patch, no? might inconvenience the author but it is the responsibility of the author and reviewer to make sure the patch didn't break anything. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 12:19 PM, Ojan Vafai o...@chromium.org wrote: On Tue, Dec 11, 2012 at 12:17 PM, Emil A Eklund e...@chromium.org wrote: I'll have to disagree with you here. If the build is broken and the gardener/build cop has a strong reason to suspect that it was caused by a specific patch and the author is unavailable then rolling that patch out is the right thing to do. It author is unavailable is the key statement here. Indeed. That said, if your strong reason turned out to be incorrect, you should recommit the patch, no? That seems like a bad idea, someone that understands the patch should recommit it. Ideally the original author. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 1:11 PM, Emil A Eklund e...@chromium.org wrote: On Tue, Dec 11, 2012 at 12:19 PM, Ojan Vafai o...@chromium.org wrote: On Tue, Dec 11, 2012 at 12:17 PM, Emil A Eklund e...@chromium.org wrote: I'll have to disagree with you here. If the build is broken and the gardener/build cop has a strong reason to suspect that it was caused by a specific patch and the author is unavailable then rolling that patch out is the right thing to do. It author is unavailable is the key statement here. Indeed. That said, if your strong reason turned out to be incorrect, you should recommit the patch, no? That seems like a bad idea, someone that understands the patch should recommit it. Ideally the original author. If it needs manual patching then you need to include the original author, but otherwise, I don't see why. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 1:11 PM, Emil A Eklund e...@chromium.org wrote: That said, if your strong reason turned out to be incorrect, you should recommit the patch, no? That seems like a bad idea, someone that understands the patch should recommit it. Ideally the original author. I don't understand your logic. A patch landed, the sheriff thinks maybe it was bad and rolls it out, then it turns out it was a red herring. Why is it not now the sheriff's responsibility to re-land? After all, the patch was landed originally by people who understood it and hasn't been seen to cause any problems. On the occasions when I've had to roll-out to diagnose an issue, I've always re-landed patches that it turns out weren't broken. Not doing this seems not only extremely rude but actively dangerous to the health of the tree, since other changes may now be landed or near-landing that depend on this change. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 1:14 PM, Peter Kasting pkast...@chromium.org wrote: On Tue, Dec 11, 2012 at 1:11 PM, Emil A Eklund e...@chromium.org wrote: That said, if your strong reason turned out to be incorrect, you should recommit the patch, no? That seems like a bad idea, someone that understands the patch should recommit it. Ideally the original author. I don't understand your logic. A patch landed, the sheriff thinks maybe it was bad and rolls it out, then it turns out it was a red herring. Why is it not now the sheriff's responsibility to re-land? After all, the patch was landed originally by people who understood it and hasn't been seen to cause any problems. There might very well have been other changes that conflicts with it. If it applies cleanly then I agree with you that whoever rolled it out should reland it. If there are conflicts or if it requires merging in any way though I'd argue that the original author needs to get involved. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 1:19 PM, Emil A Eklund e...@chromium.org wrote: I don't understand your logic. A patch landed, the sheriff thinks maybe it was bad and rolls it out, then it turns out it was a red herring. Why is it not now the sheriff's responsibility to re-land? After all, the patch was landed originally by people who understood it and hasn't been seen to cause any problems. There might very well have been other changes that conflicts with it. If it applies cleanly then I agree with you that whoever rolled it out should reland it. If there are conflicts or if it requires merging in any way though I'd argue that the original author needs to get involved. There are certainly cases where the original author needs to be involved, but I'd be happy just saying this is a judgment call. Usually rollouts happen not long after a patch lands, and roll-ins happen not long after that. In those cases, most merge failures are trivial and mechanical and can easily be handled by a conscientious sheriff who reads the relevant changes involved in the conflicts. Sometimes, of course, that's not true. But sheriffs should be biased towards try to leave working patches in the tree. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 12:17 PM, Emil A Eklund e...@chromium.org wrote: If the build is broken and the gardener/build cop has a strong reason to suspect that it was caused by a specific patch and the author is unavailable then rolling that patch out is the right thing to do. Sure. If the author isn't available via IRC emails within a reasonable time, and the regression is as serious as a build failure, then rolling out the patch is quite reasonable even if it's speculative. On the other hand, if the patch being rolled out turned out be not the cause of whatever failure the person rolled it out for, then it should be his/her responsibility to re-land the patch. It might inconvenience the author but it is the responsibility of the author and reviewer to make sure the patch didn't break anything. Sure. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
I don't understand why anyone is _speculatively_ rolling out patches. You should only be rolling it out if you _know_ the patch is bad. That said if you do rollout a random unrelated patch it is obviously your job to roll it back in. You can't say i thought this broke something, but i was wrong. Here you can have that bug again. There is no case where the original author needs to be involved as we've already determined that they did nothing wrong - the original breakage (of whatever form) was not caused by the patch you selected randomly, and they were not the author responsible for landing anything (eg. the rollout). --Oliver On Dec 11, 2012, at 1:21 PM, Peter Kasting pkast...@chromium.org wrote: On Tue, Dec 11, 2012 at 1:19 PM, Emil A Eklund e...@chromium.org wrote: I don't understand your logic. A patch landed, the sheriff thinks maybe it was bad and rolls it out, then it turns out it was a red herring. Why is it not now the sheriff's responsibility to re-land? After all, the patch was landed originally by people who understood it and hasn't been seen to cause any problems. There might very well have been other changes that conflicts with it. If it applies cleanly then I agree with you that whoever rolled it out should reland it. If there are conflicts or if it requires merging in any way though I'd argue that the original author needs to get involved. There are certainly cases where the original author needs to be involved, but I'd be happy just saying this is a judgment call. Usually rollouts happen not long after a patch lands, and roll-ins happen not long after that. In those cases, most merge failures are trivial and mechanical and can easily be handled by a conscientious sheriff who reads the relevant changes involved in the conflicts. Sometimes, of course, that's not true. But sheriffs should be biased towards try to leave working patches in the tree. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 2:14 PM, Oliver Hunt oli...@apple.com wrote: I don't understand why anyone is _speculatively_ rolling out patches. You should only be rolling it out if you _know_ the patch is bad. Sometimes something bad happens to the tree, the sheriff doesn't know which patch is responsible, and the change authors are not present to ask for help. In a case like this the sheriff has to either do speculative rollouts or leave the tree broken. Ideally, of course, change authors are around when something like this happens. But maybe the bustage doesn't happen until much later, due to some subtle/latent issue, or maybe the change author is in fact irresponsible. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Dec 11, 2012, at 2:17 PM, Peter Kasting pkast...@chromium.org wrote: On Tue, Dec 11, 2012 at 2:14 PM, Oliver Hunt oli...@apple.com wrote: I don't understand why anyone is _speculatively_ rolling out patches. You should only be rolling it out if you _know_ the patch is bad. Sometimes something bad happens to the tree, the sheriff doesn't know which patch is responsible, and the change authors are not present to ask for help. In a case like this the sheriff has to either do speculative rollouts or leave the tree broken. Ideally, of course, change authors are around when something like this happens. But maybe the bustage doesn't happen until much later, due to some subtle/latent issue, or maybe the change author is in fact irresponsible. Or the sheriff could actually see if rolling out a patch locally fixes the problem. I'm not sure why they're considering not testing to be a valid behaviour for someone who is ostensibly meant to be keeping things going in the face of people who aren't testing. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 2:20 PM, Oliver Hunt oli...@apple.com wrote: Or the sheriff could actually see if rolling out a patch locally fixes the problem. I'm not sure why they're considering not testing to be a valid behaviour for someone who is ostensibly meant to be keeping things going in the face of people who aren't testing. If the sheriff is capable of testing locally, that's an option. It's often impossible, however, for the sheriff to test locally, e.g. if the bustage is in a port he can't build. Even when possible, it may take a prohibitively long time to sync, build, and test, during which time the tree is broken for everyone. Cycling the main waterfall itself may inconvenience the rest of the developer community less. As usual, it's a judgment call. Again, I've spent many days as WebKit sheriff, and I've only done speculative rollouts a couple of times, so I don't see this as a constant, major problem. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
The build cop / gardener / sheriff / whatever may not have local or easy access to a bot that reproduces the problem ... rolling it out might be the only feasible way to test in that case. On Tue, Dec 11, 2012 at 2:20 PM, Oliver Hunt oli...@apple.com wrote: On Dec 11, 2012, at 2:17 PM, Peter Kasting pkast...@chromium.org wrote: On Tue, Dec 11, 2012 at 2:14 PM, Oliver Hunt oli...@apple.com wrote: I don't understand why anyone is _speculatively_ rolling out patches. You should only be rolling it out if you _know_ the patch is bad. Sometimes something bad happens to the tree, the sheriff doesn't know which patch is responsible, and the change authors are not present to ask for help. In a case like this the sheriff has to either do speculative rollouts or leave the tree broken. Ideally, of course, change authors are around when something like this happens. But maybe the bustage doesn't happen until much later, due to some subtle/latent issue, or maybe the change author is in fact irresponsible. Or the sheriff could actually see if rolling out a patch locally fixes the problem. I'm not sure why they're considering not testing to be a valid behaviour for someone who is ostensibly meant to be keeping things going in the face of people who aren't testing. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Please avoid rolling out patches speculatively and reland them ASAP if you had to
On Tue, Dec 11, 2012 at 2:17 PM, Peter Kasting pkast...@chromium.orgwrote: On Tue, Dec 11, 2012 at 2:14 PM, Oliver Hunt oli...@apple.com wrote: I don't understand why anyone is _speculatively_ rolling out patches. You should only be rolling it out if you _know_ the patch is bad. Sometimes something bad happens to the tree, the sheriff doesn't know which patch is responsible, and the change authors are not present to ask for help. In a case like this the sheriff has to either do speculative rollouts or leave the tree broken. Right. Ideally, of course, change authors are around when something like this happens. But maybe the bustage doesn't happen until much later, due to some subtle/latent issue, or maybe the change author is in fact irresponsible. Given that some bots take 4-5 hours to cycle these days, it's hard to keep eyes on all bots all the time. So things like this would happen. Having said that, a *speculative *roll out should one's last report. Rolling out a patch causes a lot of svn churn, increases bot cycle time, etc... and should be avoided if the failure can be fixed easily. Furthermore, it's often not too hard to test a rollout locally to see if it actually fixes the problem as Oliver suggested. - In general, I feel that some people are too religious about keeping bots green and too eager to roll out patches without trying to fix the failures or even understanding the failures and are actively harmful to the project. The main goal of continus build test systems should be to help the development of WebKit, not to run them for the sake of keeping them green. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo/webkit-dev
