Shubhendu Tripathi has posted comments on this change. Change subject: gluster: Sync job for gluster volume snapshots ......................................................................
Patch Set 6: (8 comments) http://gerrit.ovirt.org/#/c/35904/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSnapshotSyncJob.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSnapshotSyncJob.java: Line 71: if (!supportsGlusterSnapshotFeature(cluster)) { Line 72: return; Line 73: } Line 74: Line 75: final VDS upServer = getClusterUtils().getRandomUpServer(cluster.getId()); > It could be null if there are no up servers will add null check Line 76: List<GlusterVolumeEntity> volumes = getGlusterVolumeDao().getByClusterId(cluster.getId()); Line 77: Line 78: if (volumes != null && volumes.size() > 0) { Line 79: List<Callable<Pair<GlusterVolumeEntity, VDSReturnValue>>> taskList = Line 146: for (final GlusterVolumeSnapshotEntity fetchedSnapshot : fetchedSnapshots) { Line 147: GlusterVolumeSnapshotEntity existingSnapshot = existingSnapshotsMap.get(fetchedSnapshot.getId()); Line 148: Line 149: if (existingSnapshot != null) { Line 150: existingSnapshot.setStatus(fetchedSnapshot.getStatus()); > You can check for status change then update in db if required done Line 151: updatedSnapshots.add(existingSnapshot); Line 152: } else { Line 153: newlyAddedSnapshots.add(fetchedSnapshot); Line 154: } Line 175: } Line 176: Line 177: private void addOrUpdateSnapshotsConfig(Guid clusterId, GlusterSnapshotConfigInfo configInfo) { Line 178: try (EngineLock lock = acquireEntityLock(clusterId)) { Line 179: for (String key : configInfo.getClusterConfigOptions().keySet()) { > You could use Map.Entry done Line 180: String value = configInfo.getClusterConfigOptions().get(key); Line 181: if (value != null) { Line 182: addOrUpdateClusterConfig(clusterId, key, value); Line 183: } Line 185: } Line 186: Line 187: Map<String, Map<String, String>> volumeConfigs = configInfo.getVolumeConfigOptions(); Line 188: for (String key : volumeConfigs.keySet()) { Line 189: GlusterVolumeEntity volume = getGlusterVolumeDao().getByName(clusterId, key); > This might return null if the volume is not present in db will add null check Line 190: try (EngineLock lock = acquireEntityLock(volume.getId())) { Line 191: Map<String, String> volumeConfig = volumeConfigs.get(key); Line 192: if (volumeConfig != null) { Line 193: for (String configKey : volumeConfig.keySet()) { Line 214: getGlusterVolumeSnapshotConfigDao().save(param); Line 215: } else { Line 216: getGlusterVolumeSnapshotConfigDao().updateConfigByClusterIdAndName(clusterId, Line 217: paramName, Line 218: paramValue); > You can update only if the value has changed will add value check and then update Line 219: } Line 220: } Line 221: Line 222: private void addOrUpdateVolumeConfig(Guid clusterId, Guid volumeId, String paramName, String paramValue) { Line 234: } else { Line 235: getGlusterVolumeSnapshotConfigDao().updateConfigByVolumeIdIdAndName(clusterId, Line 236: volumeId, Line 237: paramName, Line 238: paramValue); > same here done Line 239: } Line 240: } Line 241: Line 242: private void saveNewSnapshots(List<GlusterVolumeSnapshotEntity> snaphosts) { http://gerrit.ovirt.org/#/c/35904/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeSnapshotConfigReturnForXmlRpc.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeSnapshotConfigReturnForXmlRpc.java: Line 24: super(innerMap); Line 25: status = new StatusForXmlRpc((Map<String, Object>) innerMap.get(STATUS)); Line 26: Line 27: Map<String, Object> systemConfig = (Map<String, Object>) innerMap.get(SYSTEM_CONFIG); Line 28: glusterSnapshotConfigInfo.setClusterId(clusterId); > I don't see clusterId being used in sync-job. Is this required? will check that and change accordingly Line 29: Map<String, String> clusterConfigs = new HashMap<String, String>(); Line 30: for (String key : systemConfig.keySet()) { Line 31: String value = (String) systemConfig.get(key); Line 32: clusterConfigs.put(key, value == null ? "" : value); http://gerrit.ovirt.org/#/c/35904/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeSnapshotInfoReturnForXmlRpc.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeSnapshotInfoReturnForXmlRpc.java: Line 64: } catch (Exception e) { Line 65: log.error("Could not populate snapshots of volume '{}' on cluster '{}': {}", Line 66: volume.getName(), Line 67: volume.getClusterId(), Line 68: e.getMessage()); > This is just a failure in parsing the timestamp and it doesn't affect much. ok. will try changing it Line 69: log.debug("Exception", e); Line 70: } Line 71: newSnapshot.setSnapshotId(Guid.createGuidFromString((String) snapshotInfo.get(UUID))); Line 72: newSnapshot.setSnapshotName((String) snapshotInfo.get(NAME)); -- To view, visit http://gerrit.ovirt.org/35904 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7b7bf79b72fc5680dab301b290e7aa860d5c714d Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
