Yair Zaslavsky has posted comments on this change.
Change subject: core:WIP: introducing OvfAutoUpdate
......................................................................
Patch Set 3: I would prefer that you didn't submit this
(10 inline comments)
And another comment - these new long fields - shouldn't they participate at
equals() method?
....................................................
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;
Why do you have these two last lines?
Why not select fn_db_add_column('vm_static', 'ovf_generation', 'BIGINT default
1') ?
And why bigint and not integer?
....................................................
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');
Please keep alpha-betical order here.
Missing handling in engine-config (i.e - engine-config.properties)
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/InitBackendServicesOnStartupBean.java
Line 37: log.infoFormat("InitResourceManager: {0}", new Date());
Line 38: ResourceManager.getInstance().init();
Line 39: AsyncTaskManager.getInstance().InitAsyncTaskManager();
Line 40: log.infoFormat("AsyncTaskManager: {0}", new Date());
Line 41: OvfDataUpdater.getInstance().InitOvfDataUpdater();
hmm, no need to use C# coding style here. should be initXXX and not InitXXXX
Line 42: log.infoFormat("OvfDataUpdater: {0}", new Date());
Line 43:
Line 44: if (Config.<Boolean>
GetValue(ConfigValues.EnableVdsLoadBalancing)) {
Line 45: VdsLoadBalancer.EnableLoadBalancer();
....................................................
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();
I really prefer you move this to the init method.
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 45:
Line 46: @OnTimerMethodAnnotation("ovfUpdate_timer")
Line 47: public void ovfUpdate_timer() {
Line 48: log.info("OvfDataUpdater: Attempting to update VMs/Templates
Ovf.");
Line 49: List<storage_pool> storagePools =
DbFacade.getInstance().getStoragePoolDao().getAll();
Please use getStoragePoolDao() convention (i.e - add a method of
getStoragePoolDao() { return DbFacade.getInstance().getStoragePoolDao(); }
Line 50: for (storage_pool pool : storagePools) {
Line 51: try {
Line 52: if (StoragePoolStatus.Up == pool.getstatus()) {
Line 53: log.infoFormat("OvfDataUpdater: Attempting to
update VMs/Templates Ovf in Data Center {0}",
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);
Question - is this the only place that is going to call updateVmInSpm from now
on? if so, why have this logic at VmCommand and not move it here?
Line 60: if (!vmsForUpdate.isEmpty()) {
Line 61: for (VM vm : vmsForUpdate) {
Line 62: DbFacade.getInstance()
Line 63: .getVmDao()
Line 78: }
Line 79: } catch (Exception ex) {
Line 80: addAuditLogError(pool.getname());
Line 81: log.errorFormat("Exception while trying to update
VMs/Templates ovf in Data Center {0}, the exception is {1}",
Line 82: pool.getname(),
I would appreciate if besides this, you also add log.debug with the stackTrace
- I know we don't keep this as "logging pattern". IMHO - we should
Line 83: ex.getMessage());
Line 84: }
Line 85: }
Line 86: }
Line 131: * @return
Line 132: */
Line 133: private boolean verifyDisksNotLocked (List<DiskImage> disks) {
Line 134: for (DiskImage disk : disks) {
Line 135: if (disk.getimageStatus() == ImageStatus.LOCKED) {
Minor comment - you're a bit inconsistent in how you're performing equation of
enums - do you prefer the literal to be at left hand side (you did that
previously at this file).
Line 136: return false;
Line 137: }
Line 138: }
Line 139: return true;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java
Line 266: if (getVm() != null) {
Line 267: if (getVm().getstatus() == VMStatus.ImageLocked) {
Line 268: VmHandler.unlockVm(getVm(), getCompensationContext());
Line 269: }
Line 270: throw new RuntimeException("asd");
Unmeaningful exception - "asd" will not really help us at the logs :)
Line 271: } else {
Line 272: setCommandShouldBeLogged(false);
Line 273: log.warn("VmCommand::EndVmCommand: Vm is null - not
performing EndAction on Vm");
Line 274: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBase.java
Line 112: @Column(name = "db_generation")
Line 113: private long db_generation;
Line 114:
Line 115: @Column(name = "ovf_generation")
Line 116: private long ovf_generation;
s/ovf_generation/ovfGeneration
Please use proper java convention. I don't like our old C# convention. I now
understand why you used bigint at db. Hibernate maps bigint to long (for
example). but why do you want to use long to begin with?
Line 117:
Line 118: @Column(name = "is_smartcard_enabled")
Line 119: private boolean smartcardEnabled;
Line 120:
--
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 <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
