----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15932/#review29591 -----------------------------------------------------------
plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java <https://reviews.apache.org/r/15932/#comment57191> 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()); plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java <https://reviews.apache.org/r/15932/#comment56987> By coding standards, please remove the tab characters. plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtStoragePoolDef.java <https://reviews.apache.org/r/15932/#comment57192> Remove the string concatenations from the StringBuilder appends to avoid needless string allocation that StringBuilder is intended to avoid. plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java <https://reviews.apache.org/r/15932/#comment57193> The log level of this message should be a WARN to let operators know that the state of the underlying storage may be out of sync with the CloudStack configuration. plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java <https://reviews.apache.org/r/15932/#comment57194> 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. plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/storage/LibvirtStorageAdaptor.java <https://reviews.apache.org/r/15932/#comment57195> Lines 518 and 519 can be collapsed to the following: s_logger.error("Failed to create mount.", e). Also, please include some state information (e.g. mount name, point, etc) in the error message so that an operator can correlate the error with the CloudStack configuration and/or the underlying storage device. plugins/storage/volume/default/src/org/apache/cloudstack/storage/datastore/lifecycle/CloudStackPrimaryDataStoreLifeCycleImpl.java <https://reviews.apache.org/r/15932/#comment57196> Magic value. This default port value should be captured somewhere in the Gluster driver as a constant. - John Burwell On Dec. 1, 2013, 9:07 a.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, 9:07 a.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 > >
