Moti Asayag has posted comments on this change. Change subject: core: [RFE]Backup Awareness - Handler ......................................................................
Patch Set 1: (3 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; > so ??? since the eclipse formatter is the formal formatter of the ovirt project as stated on [1], pushing a file which is not following the formatter rules - violates that formatter rule. The implication are on future changes to this patch by developers which uses the eclipse formatter which will eventually ends up with a new reorder for the entire imports - so even a minor change to this file will result in modifying the 20 lines of the imports unnecessarily. [1] http://www.ovirt.org/Building_oVirt_Engine/IDE 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 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"; > No, it is not configurable , we have the db_name just to enable backup awar i don't think hard-coding the db engine name is a good practice. See the definition of the datasource in packaging/services/ovirt-engine/ovirt-engine.xml.in which its default value is 'engine' (see packaging/services/ovirt-engine/ovirt-engine.conf.in) but, that's only the default. The real value is taken from /etc/ovirt-engine/engine.conf.d/10-setup-database.conf Renaming default schema is at supported on dev-env and also on production, when running engine-setup: Would you like Setup to automatically configure postgresql and create Engine database, or prefer to perform that manually? (Automatic, Manual) [Automatic]: Manual So if we wish to resolve the db name, we should query the db itself for that for example. Line 30: Line 31: @Inject Line 32: private AuditLogDirector auditLogDirector; Line 33: 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) { > We do it in all handlers, I prefer consistency and I think that in this cas this is not harmful, but the overall duration of the this job is less than a second, which is the overall duration of doBackupCheck() method. I don't expect any admin to configure BackupCheckPeriodInHours, which its type is integer, in less than 1 hour (0 is meaningless), so triggering this batch on hourly basis won't get into sync issues. If you insist of having this sync block, make sure to add the 'volatile' modifier to the active variable. Line 61: log.info("Backup check started."); Line 62: active = true; Line 63: doBackupCheck(); Line 64: log.info("Backup check completed."); -- 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
