> On Dec. 4, 2013, 5:17 p.m., John Burwell wrote: > > > > Niels de Vos wrote: > Thanks John! I'll have a go at impoving the patch and will update it > (hopefully) soon. > > Amogh Vasekar wrote: > Reminder - > Hi, > This review has been pending for long. Please do update the patch so it > may be shipped soon. > Thanks!
Thanks for the reminder. I think all comments have been addressed in the next version of the patch. I have not rewritten existing parts that could get improved based on the comments (like StringBuilder). My preference is to keep this patch Gluster specific and not modify unrelated bits. Cheers, Niels > On Dec. 4, 2013, 5:17 p.m., John Burwell wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java, > > line 133 > > <https://reviews.apache.org/r/15932/diff/1/?file=392660#file392660line133> > > > > If the Gluster poolType is netfs, why isn't this rule captured in the > > enumeration? This feels like a leaky abstraction that will be difficult to > > maintain. > > > > Also, concatenating strings as part of StringBuilder append cancels out > > the performance benefits of StringBuilder. This code should be converted > > to the following: > > > > storagePoolBuilder.append("<pool type='"); > > storagePoolBuilder.append(_poolType); > > storagePoolBuilder.append("'>"); > > storagePoolBuilder.append(System.lineSeparator()); Currently libvirt can only mount glusterfs with help from the glusterfs-fuse client. In libvirt, this is configured as a "netfs"-pool with type=glusterfs. In future, libvirt will be able to use the native GlusterFS protocol (through libgfapi.so), which will result in a different type of pool. I do not see how to write the code for a netfs+glusterfs much cleaner. - StringBuilder changes have been applied. > On Dec. 4, 2013, 5:17 p.m., John Burwell wrote: > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java, > > line 362 > > <https://reviews.apache.org/r/15932/diff/1/?file=392661#file392661line362> > > > > Extract this if block to factory method on the poolType enumeration > > (e.g. toStoragePoolType). It should also include a catch all that throws a > > CloudRuntimeException for an unsupported type mapping. I'm not sure how you intend this. It seems very common to use if-else-if statements for this... > On Dec. 4, 2013, 5:17 p.m., John Burwell wrote: > > plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java, > > line 286 > > <https://reviews.apache.org/r/15932/diff/1/?file=392662#file392662line286> > > > > Magic value. This default port value should be captured somewhere in > > the Gluster driver as a constant. It follows the same concept as the existing other drivers... I'm not sure where it would be more suitable to place this. - Niels ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review29591 ----------------------------------------------------------- On Dec. 1, 2013, 3:07 p.m., Niels de Vos wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15932/ > ----------------------------------------------------------- > > (Updated Dec. 1, 2013, 3:07 p.m.) > > > Review request for cloudstack. > > > Repository: cloudstack-git > > > Description > ------- > > The support for Gluster as Primary Storage is mostly based on the > implementation for NFS. Like NFS, libvirt can address a Gluster environment > through the 'netfs' pool-type. > > > Diffs > ----- > > api/src/com/cloud/storage/Storage.java 07b6667 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java > 182cb22 > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java > e181cea > > plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java > 0760e51 > > plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java > 7555c1e > > Diff: https://reviews.apache.org/r/15932/diff/ > > > Testing > ------- > > See http://blog.nixpanic.net/2013/12/using-gluster-as-primary-storage-in.html > > > Thanks, > > Niels de Vos > >
