Yair Zaslavsky has posted comments on this change. Change subject: core:WIP: introducing OvfAutoUpdate ......................................................................
Patch Set 3: (4 inline comments) .................................................... File backend/manager/dbscripts/upgrade/03_01_1490_add_vm_generation_columns.sql Line 1: select fn_db_add_column('vm_static', 'ovf_generation', 'BIGINT'); Line 2: select fn_db_add_column('vm_static', 'db_generation', 'BIGINT default 1'); Line 3: UPDATE vm_static set ovf_generation = 1; Line 4: ALTER TABLE vm_static ALTER COLUMN ovf_generation set default 0; About my comment - 1. I think this is not clear 2. I would consult with Eli about this. .................................................... File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql Line 549: select fn_db_add_config_value('VmPriorityMaxValue','100','general'); Line 550: --Handling Keyboard Layout configuration for VNC Line 551: select fn_db_add_config_value('VncKeyboardLayout','en-us','general'); Line 552: select fn_db_add_config_value('WaitForVdsInitInSec','60','general'); Line 553: select fn_db_add_config_value('OvfUpdateIntervalInMinutes','1','general'); I know we sometimes miss this in reviews, this is a shame from our side. Line 554: --The default network connectivity check timeout Line 555: select fn_db_add_config_value('NetworkConnectivityCheckTimeoutInSeconds','120','general'); Line 556: -- AutoRecoveryConfiguration Line 557: select fn_db_add_config_value('AutoRecoveryAllowedTypes','{\"storage domains\":\"false\",\"hosts\":\"true\"}','general'); .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java Line 28: private static final Log log = LogFactory.getLog(OvfDataUpdater.class); Line 29: private static final OvfDataUpdater INSTANCE = new OvfDataUpdater(); Line 30: Line 31: private OvfDataUpdater(){ Line 32: SchedulerUtil scheduler = SchedulerUtilQuartzImpl.getInstance(); Won't happen if this is a singleton and you init it properly on startup of backend service. there are other ways to solve this issue if you really insist. Line 33: scheduler.scheduleAFixedDelayJob(this, "ovfUpdate_timer", new Class[] {}, Line 34: new Object[] {}, Config.<Integer> GetValue(ConfigValues.OvfUpdateIntervalInMinutes), Line 35: Config.<Integer> GetValue(ConfigValues.OvfUpdateIntervalInMinutes), TimeUnit.SECONDS); Line 36: } Line 55: List<VM> vmsForUpdate = Line 56: getVmsForUpdate(DbFacade.getInstance() Line 57: .getVmDao() Line 58: .getAllVmsForOvfUpdateForStoragePool(pool.getId())); Line 59: VmCommand.updateVmInSpm(pool.getId(), vmsForUpdate); Ok, I would add a comment on this patch indicating it will be handled for the sake of the review. Thanks for clarifying. Line 60: if (!vmsForUpdate.isEmpty()) { Line 61: for (VM vm : vmsForUpdate) { Line 62: DbFacade.getInstance() Line 63: .getVmDao() -- To view, visit http://gerrit.ovirt.org/9328 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9b5132300fb1f1fd94f771cab15efe5246dbeca8 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Ayal Baron <aba...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> Gerrit-Reviewer: liron aravot <liron.ara...@gmail.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches