Ramesh N has posted comments on this change. Change subject: engine: sync job for gluster disk provisioning ......................................................................
Patch Set 11: (9 comments) Patch set to follow. https://gerrit.ovirt.org/#/c/36429/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/StorageDeviceSyncJob.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/StorageDeviceSyncJob.java: Line 13: import org.ovirt.engine.core.common.businessentities.VDS; Line 14: import org.ovirt.engine.core.common.businessentities.VDSGroup; Line 15: import org.ovirt.engine.core.common.businessentities.gluster.StorageDevice; Line 16: import org.ovirt.engine.core.common.gluster.GlusterFeatureSupported; Line 17: import org.ovirt.engine.core.common.utils.ObjectUtils; > since this class on the bll module, please use java.util.Objects.equal for Done Line 18: import org.ovirt.engine.core.common.utils.Pair; Line 19: import org.ovirt.engine.core.common.vdscommands.VDSCommandType; Line 20: import org.ovirt.engine.core.common.vdscommands.VDSReturnValue; Line 21: import org.ovirt.engine.core.common.vdscommands.VdsIdVDSCommandParametersBase; Line 24: import org.ovirt.engine.core.utils.timer.OnTimerMethodAnnotation; Line 25: import org.slf4j.Logger; Line 26: import org.slf4j.LoggerFactory; Line 27: Line 28: public class StorageDeviceSyncJob extends GlusterJob { > wouldn't you rather define it as @Singleton and inject it to the commands ? Done Line 29: private static final Logger log = LoggerFactory.getLogger(StorageDeviceSyncJob.class); Line 30: Line 31: private static final StorageDeviceSyncJob instance = new StorageDeviceSyncJob(); Line 32: Line 93: Line 94: } Line 95: Line 96: private void updateStorageDevices(VDS vds, List<StorageDevice> storageDevicesFromVdsm) { Line 97: Set<String> deviceUuidsFromVdsm = new HashSet<String>(); > re-declaring the type on the right side is not required Done Line 98: Set<String> deviceNamesFromVdsm = new HashSet<String>(); Line 99: Line 100: List<StorageDevice> storageDevicesInDb = getStorageDeviceDao().getStorageDevicesInHost(vds.getId()); Line 101: Map<String, StorageDevice> nameToDeviceMap = new HashMap<String, StorageDevice>(); Line 98: Set<String> deviceNamesFromVdsm = new HashSet<String>(); Line 99: Line 100: List<StorageDevice> storageDevicesInDb = getStorageDeviceDao().getStorageDevicesInHost(vds.getId()); Line 101: Map<String, StorageDevice> nameToDeviceMap = new HashMap<String, StorageDevice>(); Line 102: Map<String, StorageDevice> deviceUuidToDeviceMap = new HashMap<String, StorageDevice>(); > same Done Line 103: Line 104: // Make deviceUuid to Device map and deviceName to device map so that we can find the Line 105: // newly added and updated devices without looping over the same list again and again. Line 106: for (StorageDevice storageDevice : storageDevicesInDb) { Line 109: deviceUuidToDeviceMap.put(storageDevice.getDevUuid(), storageDevice); Line 110: } Line 111: } Line 112: Line 113: List<StorageDevice> storageDevicesToUpdate = new ArrayList<StorageDevice>(); > and here as well Done Line 114: List<StorageDevice> storageDevicesToDelete = new ArrayList<StorageDevice>(); Line 115: Line 116: for (StorageDevice storageDevice : storageDevicesFromVdsm) { Line 117: // Create deviceName and deviceUuid set to use it while finding the deleted services. Line 177: Line 178: private void logStorageDeviceMessage(AuditLogType logType, VDS vds, final StorageDevice device) { Line 179: logUtil.logAuditMessage(vds.getVdsGroupId(), null, vds, Line 180: logType, Line 181: new HashMap<String, String>() { > can this be replaced with Collections.singletonMap() ? or is there a plan t I don't see any case to add extra value here. Changing it to singletonMap as suggested. Line 182: { Line 183: put("storageDevice", device.getName()); Line 184: } Line 185: }); https://gerrit.ovirt.org/#/c/36429/11/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/SyncStorageDevicesCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/SyncStorageDevicesCommand.java: Line 17: } Line 18: Line 19: @Override Line 20: protected boolean canDoAction() { Line 21: VDSGroup cluster = getDbFacade().getVdsGroupDao().get(getVds().getVdsGroupId()); > You can just call getVdsGroup() Done Line 22: if (!cluster.supportsGlusterService() Line 23: || !GlusterFeatureSupported.glusterDiskProvisioning(cluster.getCompatibilityVersion())) { Line 24: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_PROVISIONING_NOT_SUPPORTED_BY_CLUSTER); Line 25: } Line 44: } Line 45: Line 46: @Override Line 47: public AuditLogType getAuditLogTypeValue() { Line 48: if (getSucceeded()) { > it's a matter of styling, but this could be written as: Done Line 49: return AuditLogType.SYNC_STORAGE_DEVICES_IN_HOST; Line 50: } else { Line 51: return AuditLogType.SYNC_STORAGE_DEVICES_IN_HOST_FAILED; Line 52: } https://gerrit.ovirt.org/#/c/36429/11/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/gluster/GlusterFeatureSupported.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/gluster/GlusterFeatureSupported.java: Line 98: * Compatibility version to check for. Line 99: * @return <code>true</code> if disk provisioning feature is enabled, Line 100: * <code>false</code> if it's not. Line 101: */ Line 102: public static boolean glusterDiskProvisioning(Version version) { > call it glusterBrickProvisioning? changed the method name also the config value. Line 103: return supportedInConfig(ConfigValues.GlusterDiskProvisioningEnabled, version); Line 104: } -- To view, visit https://gerrit.ovirt.org/36429 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I651bb51873a96d491c5a5f51147cb72be958985a Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
