Sahina Bose has posted comments on this change. Change subject: gluster: BLL command for scheduling volume snapshot ......................................................................
Patch Set 3: (10 comments) http://gerrit.ovirt.org/#/c/36980/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSnapshotScheduleJob.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterSnapshotScheduleJob.java: Line 53: snapshot.setCreatedAt(new Date()); Line 54: snapshot.setStatus(GlusterSnapshotStatus.STARTED); Line 55: getGlusterVolumeSnapshotDao().save(snapshot); Line 56: } else { Line 57: log.error("Error while creating snapshot for volume '{}': {}", volume.getName(), returnValue.getVdsError() How about raising an Audit log for failure? Otherwise, it may be difficult to track which snapshots failed Line 58: .getMessage()); Line 59: log.debug(returnValue.getVdsError().getMessage()); Line 60: } Line 61: } http://gerrit.ovirt.org/#/c/36980/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/ReScheduleGlusterVolumeSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/ReScheduleGlusterVolumeSnapshotCommand.java: Line 21: import org.ovirt.engine.core.dao.gluster.GlusterVolumeSnapshotScheduleDao; Line 22: import org.ovirt.engine.core.utils.timer.DBSchedulerUtilQuartzImpl; Line 23: import org.ovirt.engine.core.utils.timer.SchedulerUtil; Line 24: Line 25: public class ReScheduleGlusterVolumeSnapshotCommand extends GlusterVolumeCommandBase<ScheduleGlusterVolumeSnapshotParameters> { Reschedule? (without capitalising S) Line 26: private GlusterVolumeSnapshotSchedule schedule; Line 27: private Guid volumeId; Line 28: private boolean force; Line 29: Line 54: String jobId = fetchedSchedule.getJobId(); Line 55: SchedulerUtil scheduler = DBSchedulerUtilQuartzImpl.getInstance(); Line 56: Line 57: // delete the existing job Line 58: scheduler.deleteJob(jobId); How about handling exceptions? Line 59: Line 60: if (schedule.getRecurrence() != null) { Line 61: // create a new job with same id Line 62: String cronExpression = GlusterUtil.getInstance().getCronExpression(schedule); Line 66: new Object[] { upServer.getId().toString(), volumeId.toString(), schedule.getSnapshotNamePrefix(), Line 67: schedule.getSnapshotDescription(), force }, Line 68: cronExpression); Line 69: Line 70: if (newJobId != null) { if newJobId is null or exception in scheduling? Line 71: setSucceeded(true); Line 72: schedule.setJobId(newJobId); Line 73: getGlusterVolumeSnapshotScheduleDao().updateScheduleByVolumeId(volumeId, schedule); Line 74: } Line 94: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_DOES_NOT_SUPPORT_GLUSTER); Line 95: return false; Line 96: } Line 97: Line 98: GlusterVolumeEntity volume = getGlusterVolumeDao().getById(volumeId); you can use getGlusterVolume() and the above checks are already done in the super class Line 99: if (volume != null && volume.getStatus() == GlusterStatus.DOWN) { Line 100: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_IS_DOWN); Line 101: } Line 102: Line 101: } Line 102: Line 103: if (!GlusterUtil.getInstance().isVolumeThinlyProvisioned(volume)) { Line 104: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_IS_NOT_THINLY_PROVISIONED); Line 105: } How about extraction all of the common logic to a GlusterVolumeSnapshotBaseCommand? You will also need to check that Volume snapshotting is supported at this cluster level Line 106: Line 107: GlusterVolumeSnapshotSchedule fetchedSchedule = getGlusterVolumeSnapshotScheduleDao().getByVolumeId(volumeId); Line 108: if (fetchedSchedule == null) { Line 109: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_NOT_SCHEDULED); Line 106: Line 107: GlusterVolumeSnapshotSchedule fetchedSchedule = getGlusterVolumeSnapshotScheduleDao().getByVolumeId(volumeId); Line 108: if (fetchedSchedule == null) { Line 109: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_SNAPSHOT_NOT_SCHEDULED); Line 110: } Can you not reschedule if previous schedule is null? Line 111: Line 112: return true; Line 113: } Line 114: Line 115: @Override Line 116: protected Map<String, Pair<String, String>> getExclusiveLocks() { Line 117: if (!isInternalExecution()) { Line 118: return Collections.singletonMap(schedule.getVolumeId().toString(), Line 119: LockMessagesMatchUtil.makeLockingPair(LockingGroup.GLUSTER, LockingGroup GLUSTER or VOLUME_SNAPSHOT? Again, you could move this to a common base class as all snapshot commands would have the same logic Line 120: VdcBllMessages.ACTION_TYPE_FAILED_OBJECT_LOCKED)); Line 121: } Line 122: return null; Line 123: } http://gerrit.ovirt.org/#/c/36980/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/ScheduleGlusterVolumeSnapshotCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/ScheduleGlusterVolumeSnapshotCommand.java: Line 20: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 21: import org.ovirt.engine.core.dao.gluster.GlusterVolumeSnapshotScheduleDao; Line 22: import org.ovirt.engine.core.utils.timer.DBSchedulerUtilQuartzImpl; Line 23: Line 24: public class ScheduleGlusterVolumeSnapshotCommand extends GlusterVolumeCommandBase<ScheduleGlusterVolumeSnapshotParameters> { Can Reschedule extend this? or is there a need for reschedule if you take care of updating existing schedule here? Line 25: private GlusterVolumeSnapshotSchedule schedule; Line 26: private Guid volumeId; Line 27: private boolean force; Line 28: http://gerrit.ovirt.org/#/c/36980/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/GlusterUtil.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/GlusterUtil.java: Line 283: Line 284: switch (schedule.getRecurrence()) { Line 285: case INTERVAL: Line 286: int interval = schedule.getInterval(); Line 287: retStr = "0 */" + interval + " * * * ? *"; use String.format here and below Line 288: break; Line 289: case HOURLY: Line 290: // Date startDate = schedule.getStartDate(); Line 291: retStr = "0 0 * * * ? *"; -- To view, visit http://gerrit.ovirt.org/36980 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4326ebb0c146eadceb6ae30cce73ece132749dc5 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@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
