Eli Mesika has posted comments on this change. Change subject: core: [RFE]Backup Awareness - Handler ......................................................................
Patch Set 1: (11 comments) https://gerrit.ovirt.org/#/c/39527/1/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EngineBackupAwarenessManager.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/EngineBackupAwarenessManager.java: Line 11: import org.ovirt.engine.core.utils.timer.SchedulerUtilQuartzImpl; Line 12: import org.slf4j.Logger; Line 13: import org.slf4j.LoggerFactory; Line 14: Line 15: import javax.inject.Inject; > the eclipse formatter tends to organize imports by lexicographic order. so ??? Line 16: import javax.inject.Singleton; Line 17: import java.util.Calendar; Line 18: import java.util.Date; Line 19: import java.util.concurrent.TimeUnit; Line 24: */ Line 25: @Singleton Line 26: public class EngineBackupAwarenessManager { Line 27: private static final Logger log = LoggerFactory.getLogger(EngineBackupAwarenessManager.class); Line 28: private boolean active = false; > false is the default for boolean. will fix Line 29: private static final String DB = "engine"; Line 30: Line 31: @Inject Line 32: private AuditLogDirector auditLogDirector; Line 25: @Singleton Line 26: public class EngineBackupAwarenessManager { Line 27: private static final Logger log = LoggerFactory.getLogger(EngineBackupAwarenessManager.class); Line 28: private boolean active = false; Line 29: private static final String DB = "engine"; > s/DB/DB_NAME No, it is not configurable , we have the db_name just to enable backup awareness to the history db as well in future. I see no reason to support more than one engine DB, we have no such usecase Line 30: Line 31: @Inject Line 32: private AuditLogDirector auditLogDirector; Line 33: Line 34: /** Line 35: * Initializes the backup h Check Manager Line 36: */ Line 37: public void initialize() { Line 38: if(Config.<Boolean>getValue(ConfigValues.BackupCheckPeriodInHours)) { > how can you refer to BackupCheckPeriodInHours as a boolean ? This will alwa will fix No need for feature enabled for that , this is not a cluster level feature which depends on version Line 39: log.info("Start initializing {}", getClass().getSimpleName()); Line 40: Integer backupCheckPeriodInHours = Config.<Integer> getValue(ConfigValues.BackupCheckPeriodInHours); Line 41: // disable feature if value is negative Line 42: if (backupCheckPeriodInHours > 0) { Line 54: Line 55: @OnTimerMethodAnnotation("backupCheck") Line 56: public void backupCheck() { Line 57: // skip backup check if previous operation is not completed yet Line 58: if (!active) { > why do we need the active variable ? See the comment above // skip backup check if previous operation is not completed yet Line 59: try { Line 60: synchronized (this) { Line 61: log.info("Backup check started."); Line 62: active = true; Line 56: public void backupCheck() { Line 57: // skip backup check if previous operation is not completed yet Line 58: if (!active) { Line 59: try { Line 60: synchronized (this) { > this is expected to be invoked on hourly basis. The entire duration of the We do it in all handlers, I prefer consistency and I think that in this case we lose nothing in doing it the way other handlers are working and BTW, in large systems even if it should take 1 sec , it can wait long minutes in the queue if DB is busy Line 61: log.info("Backup check started."); Line 62: active = true; Line 63: doBackupCheck(); Line 64: log.info("Backup check completed."); Line 57: // skip backup check if previous operation is not completed yet Line 58: if (!active) { Line 59: try { Line 60: synchronized (this) { Line 61: log.info("Backup check started."); > this should be in debug level imo We are puting this as INFO in other handlers Line 62: active = true; Line 63: doBackupCheck(); Line 64: log.info("Backup check completed."); Line 65: } Line 60: synchronized (this) { Line 61: log.info("Backup check started."); Line 62: active = true; Line 63: doBackupCheck(); Line 64: log.info("Backup check completed."); > same here. same as well Line 65: } Line 66: } Line 67: finally { Line 68: active = false; Line 70: } Line 71: } Line 72: Line 73: private void doBackupCheck() { Line 74: final int DAY_IN_MILLISEC= 60 * 60 * 24 *1000; > please use TimeUnit.DAYS.toMillis() sure Line 75: AuditLogableBase alert = new AuditLogableBase(); Line 76: Line 77: //try to get last backup record Line 78: EngineBackupHistory lastBackup = DbFacade.getInstance().getEngineBackupHistoryDAO().getLastSuccessfulEngineBackup(DB); Line 74: final int DAY_IN_MILLISEC= 60 * 60 * 24 *1000; Line 75: AuditLogableBase alert = new AuditLogableBase(); Line 76: Line 77: //try to get last backup record Line 78: EngineBackupHistory lastBackup = DbFacade.getInstance().getEngineBackupHistoryDAO().getLastSuccessfulEngineBackup(DB); > please replace yea , should start to get used to the @Inject Line 79: if (lastBackup == null) { Line 80: auditLogDirector.log(alert, AuditLogType.ENGINE_NO_BACKUP); Line 81: } Line 82: else { Line 77: //try to get last backup record Line 78: EngineBackupHistory lastBackup = DbFacade.getInstance().getEngineBackupHistoryDAO().getLastSuccessfulEngineBackup(DB); Line 79: if (lastBackup == null) { Line 80: auditLogDirector.log(alert, AuditLogType.ENGINE_NO_BACKUP); Line 81: } > please format the code. shoudl be will do Line 82: else { Line 83: //check time elapsed from last backup Line 84: if (Config.<Boolean>getValue(ConfigValues.BackupAlertPeriodInDays)) { Line 85: Integer backupAlertPeriodInDays = Config.<Integer> getValue(ConfigValues.BackupAlertPeriodInDays); -- To view, visit https://gerrit.ovirt.org/39527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8567ae314a3d612d8e3d4dd948394b65789ac670 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eli Mesika <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Moti Asayag <[email protected]> Gerrit-Reviewer: Oved Ourfali <[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
