Shubhendu Tripathi has posted comments on this change. Change subject: gluster: BLL command for scheduling volume snapshot ......................................................................
Patch Set 9: (9 comments) https://gerrit.ovirt.org/#/c/36980/9/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 23: import org.slf4j.Logger; Line 24: import org.slf4j.LoggerFactory; Line 25: Line 26: public class GlusterSnapshotScheduleJob implements Serializable { Line 27: private static final long serialVersionUID = 1L; > generated serial version id? done Line 28: Line 29: private final static Logger log = LoggerFactory.getLogger(GlusterSnapshotScheduleJob.class); Line 30: private GlusterAuditLogUtil logUtil = GlusterAuditLogUtil.getInstance(); Line 31: Line 26: public class GlusterSnapshotScheduleJob implements Serializable { Line 27: private static final long serialVersionUID = 1L; Line 28: Line 29: private final static Logger log = LoggerFactory.getLogger(GlusterSnapshotScheduleJob.class); Line 30: private GlusterAuditLogUtil logUtil = GlusterAuditLogUtil.getInstance(); > create a method getLogUtil - so that you can mock in test. done Line 31: Line 32: public GlusterSnapshotScheduleJob() { Line 33: } Line 34: Line 36: public void onTimer(String serverId, String volumeId, String snapshotNamePrefix, String description, boolean force) { Line 37: final GlusterVolumeEntity volume = getGlusterVolumeDao().getById(new Guid(volumeId)); Line 38: Line 39: GlusterVolumeSnapshotEntity snapshot = new GlusterVolumeSnapshotEntity(); Line 40: snapshot.setClusterId(volume.getClusterId()); > check if volume is null and log and return. done Line 41: snapshot.setVolumeId(new Guid(volumeId)); Line 42: DateFormat df = new SimpleDateFormat("yyyyMMddHHmmss"); Line 43: final String snapshotName = snapshotNamePrefix + "-snap-" + df.format(new Date()); Line 44: snapshot.setSnapshotName(snapshotName); Line 37: final GlusterVolumeEntity volume = getGlusterVolumeDao().getById(new Guid(volumeId)); Line 38: Line 39: GlusterVolumeSnapshotEntity snapshot = new GlusterVolumeSnapshotEntity(); Line 40: snapshot.setClusterId(volume.getClusterId()); Line 41: snapshot.setVolumeId(new Guid(volumeId)); > volume.getId() done Line 42: DateFormat df = new SimpleDateFormat("yyyyMMddHHmmss"); Line 43: final String snapshotName = snapshotNamePrefix + "-snap-" + df.format(new Date()); Line 44: snapshot.setSnapshotName(snapshotName); Line 45: snapshot.setDescription(description); Line 49: if (!returnValue.getSucceeded()) { Line 50: log.error("Error while creating snapshot for volume '{}': {}", Line 51: volume.getName(), Line 52: returnValue.getExecuteFailedMessages().toString()); Line 53: log.debug(returnValue.getExecuteFailedMessages().toString()); > log.debug not required here as no further information logged done Line 54: logUtil.logAuditMessage(volume.getClusterId(), Line 55: volume, Line 56: null, Line 57: AuditLogType.GLUSTER_VOLUME_SNAPSHOT_CREATE_FAILED, https://gerrit.ovirt.org/#/c/36980/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/ScheduleGlusterVolumeSnapshotCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/ScheduleGlusterVolumeSnapshotCommandBase.java: Line 68: new Class[] { String.class, String.class, String.class, String.class, Boolean.class }, Line 69: new Object[] { upServer.getId().toString(), getGlusterVolumeId().toString(), Line 70: schedule.getSnapshotNamePrefix(), Line 71: schedule.getSnapshotDescription(), force }, Line 72: cronExpression, schedule.getStartDate(), schedule.getEndByDate()); > What about the start date and end date - shouldn't these be in engine time The Date type is internally maintained as no of millis from epoch as a long number. From UI itself we are setting a Date type so I dont think we need to convert these fields. Conversion is required only if we need to display as a String to end user in readable format. In that case formatter takes a timezone as parameter. Line 73: } Line 74: protected GlusterVolumeSnapshotScheduleDao getGlusterVolumeSnapshotScheduleDao() { Line 75: return DbFacade.getInstance().getGlusterVolumeSnapshotScheduleDao(); Line 76: } Line 75: return DbFacade.getInstance().getGlusterVolumeSnapshotScheduleDao(); Line 76: } Line 77: Line 78: protected GlusterVolumeSnapshotSchedule getSchedule() { Line 79: return this.schedule; > return schedule; - should suffice done Line 80: } Line 81: Line 82: protected boolean getForce() { Line 83: return this.force; Line 79: return this.schedule; Line 80: } Line 81: Line 82: protected boolean getForce() { Line 83: return this.force; > same as above done Line 84: } https://gerrit.ovirt.org/#/c/36980/9/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 318: calFrom.set(Calendar.HOUR, inTime.getHours()); Line 319: calFrom.set(Calendar.MINUTE, inTime.getMinutes()); Line 320: calFrom.set(Calendar.SECOND, inTime.getSeconds()); Line 321: Line 322: Calendar calTo = new GregorianCalendar(TimeZone.getTimeZone(Config.<String> getValue(ConfigValues.DefaultGeneralTimeZone))); > is this the engine time zone? There are only two timezone details maintained as part of vdc_options namely DefaultWindowsTimeZone and DefaultGeneralTimeZone. I felt DefaultGeneralTimeZone is the one which should be used. Line 323: calTo.setTimeInMillis(calFrom.getTimeInMillis()); Line 324: Line 325: return new Time(calTo.get(Calendar.HOUR), calTo.get(Calendar.MINUTE), calTo.get(Calendar.SECOND)); Line 326: } -- To view, visit https://gerrit.ovirt.org/36980 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4326ebb0c146eadceb6ae30cce73ece132749dc5 Gerrit-PatchSet: 9 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
