On Wed, 2014-04-09 at 11:56 -0400, Keith Schincke wrote: > Here is a quick set of reviews. > > What package and distro version includes the lvmthin man page? I think it's still in git, but it's available online.
> On line 653 of Documentation.md, you refer to "man -7 thin". This should > be "man -7 lvmthin" Good catch, thanks! > This same is done in the README.md. Never mind. I see the sym link. Yeah. This makes the github people happy. > From line 178 to 208, you are doing a lot of work to build the thin lvm > command line. Should you wrap this in a if $lvm_thinp conditional. This > will keep all the thin provisioning code and a possible vgs system call > from occurring if $lvm_thinp is not true. Actually it's all declarative, so it's safe without it. The conditional is here at 211: $lvm_lvcreate = $lvm_thinp ? { true => "${lvm_thinp_lvcreate}", default => "/sbin/lvcreate --extents 100%PVS -n ${lvm_lvname} ${lvm_vgname}", } > > Could you also update your lines 218 - 220 to this: > > $dev2 = $lvm ? { > true => "/dev/${lvm_vgname}/${lvm_lvname}"} , > default => "${dev1}", > } > > The long "if" with a short/trivial "else" is almost an "oh, by the way" > kind of statement. It can be easy to overlook. The separate conditional > can help an other reviewer follow along more easily. Yeah, I like this actually. Thanks for the idea. Branch updated (rebased): https://github.com/purpleidea/puppet-gluster/tree/feat/thinp Commit at: https://github.com/purpleidea/puppet-gluster/commit/d204fe5c4b80f0bc6d7850da6ccc90cb695c5873 Also I added a small warning if someone enables thinp but disables LVM. James > > Keith > > > On 04/09/2014 11:13 AM, James wrote: > > Okay, > > > > Here's a first draft of puppet-gluster w/ thin-p. This patch includes > > documentation updates too! (w00t!) > > > > https://github.com/purpleidea/puppet-gluster/tree/feat/thinp > > > > FYI: I'll probably rebase this branch. > > FYI: Somewhat untested. Read the commit message. > > > > Comments welcome :) > > > > I'm most interested to hear about if everyone is pleased with the way I > > run the thin-p lv command. I think this makes the most sense, but let me > > know if anyone has improvements. Also I'd love to hear about what the > > default values for the parameters should be, but that's a one line > > patch, so no rush for me. > > > > Cheers, > > James > > > > > > >
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Gluster-devel mailing list Gluster-devel@nongnu.org https://lists.nongnu.org/mailman/listinfo/gluster-devel