5 ms·

Great to hear the ZoL guys are right on this. Bravo. It also reminds me why my NAS runs Debian..

by mafro 9y ago

sureaboutthis 9y ago
It also reminds me why my NAS runs FreeBSD.
- ryao 9y ago
  The bad patch passed review by developer(s) from other platforms. Matthew Ahrens was a reviewer. This was not merged based on unilateral review by the ZFSOnLinux developers. There was also nothing Linux specific about it. That said, bugs happen. We should be putting new test cases in place to help catch such regressions in the future. If we find more ways to harden the code add against regressions of this nature as we continue our analysis, we will certainly do them too.
  - jclulow 9y ago
    Reviewers will help catch bugs, but the engineer who writes the code and seeks to integrate it is ultimately responsible for sufficient testing to avoid issues like this one.
    - oarsinsync 9y ago
      When multiple people have reviewed and approved, all of those people are jointly and equally responsible for any fallout. A reviewer needs to take their responsibility as seriously as the coder. If they don’t, it diminishes the value of having the code review. The coder, the reviewers, the approvers, they’re all in this together. It’s unfortunate that this happened, but no single person should be held accountable when there’s a process in place designed to protect against individual mistakes. It’s a shame that this skipped through still anyway, but that’s part of the nature of the resource limitation, especially with F/LOSS. There’s always a risk of this happening. The only way to reduce the risk is to contribute more resources. Blaming the coder is more likely to result in a reduction of resources, as less code gets done.
    - baq 9y ago
      au contraire, engineers should not be testing their changes. tunnel vision is a real thing - you need to be seriously experienced to be able to sidestep that.
    - ryao 9y ago
      The author is a fairly new contributor: https://github.com/zfsonlinux/zfs/commits?author=sanjeevbagewadi https://github.com/zfsonlinux/zfs/commits?author=sanjeevbage... What do you suggest that we should have done?
      - jclulow 9y ago
        I'm not sure, but I'm not deeply familiar with this part of the code. In general, I think it's good to be able to induce all of the failure cases for all of the error handling code that's being added. This can be time-consuming work, but I would argue that this is the file system -- anything less is an unacceptable risk. This quote from my boss comes to mind: Remember: you are (or should be!) always empowered as an engineer to take more time to test your work. -- http://dtrace.org/blogs/bmc/2015/09/03/software-immaculate-fetid-and-grimy/ http://dtrace.org/blogs/bmc/2015/09/03/software-immaculate-f...

Great to hear the ZoL guys are right on this. Bravo. It also reminds me why my NAS runs Debian..

by mafro 9y ago

Great to hear the ZoL guys are right on this. Bravo. It also reminds me why my NAS runs Debian..

sureaboutthis 9y ago

It also reminds me why my NAS runs FreeBSD.

ryao 9y ago
The bad patch passed review by developer(s) from other platforms. Matthew Ahrens was a reviewer. This was not merged based on unilateral review by the ZFSOnLinux developers. There was also nothing Linux specific about it. That said, bugs happen. We should be putting new test cases in place to help catch such regressions in the future. If we find more ways to harden the code add against regressions of this nature as we continue our analysis, we will certainly do them too.
- jclulow 9y ago
  Reviewers will help catch bugs, but the engineer who writes the code and seeks to integrate it is ultimately responsible for sufficient testing to avoid issues like this one.
  - oarsinsync 9y ago
    When multiple people have reviewed and approved, all of those people are jointly and equally responsible for any fallout. A reviewer needs to take their responsibility as seriously as the coder. If they don’t, it diminishes the value of having the code review. The coder, the reviewers, the approvers, they’re all in this together. It’s unfortunate that this happened, but no single person should be held accountable when there’s a process in place designed to protect against individual mistakes. It’s a shame that this skipped through still anyway, but that’s part of the nature of the resource limitation, especially with F/LOSS. There’s always a risk of this happening. The only way to reduce the risk is to contribute more resources. Blaming the coder is more likely to result in a reduction of resources, as less code gets done.
  - baq 9y ago
    au contraire, engineers should not be testing their changes. tunnel vision is a real thing - you need to be seriously experienced to be able to sidestep that.
  - ryao 9y ago
    The author is a fairly new contributor: https://github.com/zfsonlinux/zfs/commits?author=sanjeevbagewadi https://github.com/zfsonlinux/zfs/commits?author=sanjeevbage... What do you suggest that we should have done?
    - jclulow 9y ago
      I'm not sure, but I'm not deeply familiar with this part of the code. In general, I think it's good to be able to induce all of the failure cases for all of the error handling code that's being added. This can be time-consuming work, but I would argue that this is the file system -- anything less is an unacceptable risk. This quote from my boss comes to mind: Remember: you are (or should be!) always empowered as an engineer to take more time to test your work. -- http://dtrace.org/blogs/bmc/2015/09/03/software-immaculate-fetid-and-grimy/ http://dtrace.org/blogs/bmc/2015/09/03/software-immaculate-f...