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

Reply via email to