Allon Mureinik has posted comments on this change.
Change subject: core: introducing OvfAutoUpdate
......................................................................
Patch Set 14: I would prefer that you didn't submit this
(56 inline comments)
Please fix (or at least address) the inline comments.
Also, please unit-test all the new functionality.
....................................................
File backend/manager/dbscripts/storages_sp.sql
Line 98
Line 99
Line 100
Line 101
Line 102
why is this related to this patch?
....................................................
File backend/manager/dbscripts/upgrade/03_01_1530_add_vm_generation_columns.sql
Line 1: select fn_db_add_column('vm_static', 'db_generation', 'BIGINT default
1');
Rename this file.
We're already in 3.2, so it should be called
03_02_0010_add_vm_generation_columns.sql, or something like that.
Line 2:
Line 3: -- not added as foreign key so that when vm is removed, its record in
vm_ovf_generations record will stay
Line 4: CREATE TABLE vm_ovf_generations
Line 5: (
Line 3: -- not added as foreign key so that when vm is removed, its record in
vm_ovf_generations record will stay
Line 4: CREATE TABLE vm_ovf_generations
Line 5: (
Line 6: vm_guid UUID PRIMARY KEY,
Line 7: storage_pool_id UUID references storage_pool(id) ON DELETE CASCADE,
references is an SQL keyword - it should be uppercase.
Line 8: ovf_generation BIGINT default 1
Line 9: );
Line 10:
Line 11: INSERT into vm_ovf_generations (select vm.vm_guid, sp.id from
vm_static vm ,storage_pool sp, vds_groups vg where vg.storage_pool_id = sp.id
AND vm.vds_group_id = vg.vds_group_id);
Line 4: CREATE TABLE vm_ovf_generations
Line 5: (
Line 6: vm_guid UUID PRIMARY KEY,
Line 7: storage_pool_id UUID references storage_pool(id) ON DELETE CASCADE,
Line 8: ovf_generation BIGINT default 1
default is an SQL keyword - it should be uppercase.
Line 9: );
Line 10:
Line 11: INSERT into vm_ovf_generations (select vm.vm_guid, sp.id from
vm_static vm ,storage_pool sp, vds_groups vg where vg.storage_pool_id = sp.id
AND vm.vds_group_id = vg.vds_group_id);
Line 12:
Line 7: storage_pool_id UUID references storage_pool(id) ON DELETE CASCADE,
Line 8: ovf_generation BIGINT default 1
Line 9: );
Line 10:
Line 11: INSERT into vm_ovf_generations (select vm.vm_guid, sp.id from
vm_static vm ,storage_pool sp, vds_groups vg where vg.storage_pool_id = sp.id
AND vm.vds_group_id = vg.vds_group_id);
into, select, from and where are SQL keywords - they should be uppercase.
Also, please format this code so it would be more readable:
INSERT INTO vm_ovf_generations
(SELECT vm.vm_guid, sp.id
FROM vm_static vm ,storage_pool sp, vds_groups vg
WHERE vg.storage_pool_id = sp.id AND vm.vds_group_id = vg.vds_group_id);
Line 12:
Line 13: -- Only pre-existing vms should have ovf_generation set to 1, so after
we added
Line 14: -- the pre existing vms, the default should be 0.
Line 15: ALTER TABLE vm_ovf_generations ALTER COLUMN ovf_generation set default
0;
Line 11: INSERT into vm_ovf_generations (select vm.vm_guid, sp.id from
vm_static vm ,storage_pool sp, vds_groups vg where vg.storage_pool_id = sp.id
AND vm.vds_group_id = vg.vds_group_id);
Line 12:
Line 13: -- Only pre-existing vms should have ovf_generation set to 1, so after
we added
Line 14: -- the pre existing vms, the default should be 0.
Line 15: ALTER TABLE vm_ovf_generations ALTER COLUMN ovf_generation set default
0;
isn't there a fn_db_XXX function for this?
Line 16:
Line 17: CREATE INDEX IDX_vm_ovf_generations_vm_guid ON
vm_ovf_generations(vm_guid);
Line 13: -- Only pre-existing vms should have ovf_generation set to 1, so after
we added
Line 14: -- the pre existing vms, the default should be 0.
Line 15: ALTER TABLE vm_ovf_generations ALTER COLUMN ovf_generation set default
0;
Line 16:
Line 17: CREATE INDEX IDX_vm_ovf_generations_vm_guid ON
vm_ovf_generations(vm_guid);
s/IDX/idx/
Line 14: -- the pre existing vms, the default should be 0.
Line 15: ALTER TABLE vm_ovf_generations ALTER COLUMN ovf_generation set default
0;
Line 16:
Line 17: CREATE INDEX IDX_vm_ovf_generations_vm_guid ON
vm_ovf_generations(vm_guid);
Line 18: CREATE INDEX IDX_vm_ovf_generations_storage_pool_id ON
vm_ovf_generations(storage_pool_id);
s/IDX/idx/
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 339: select
fn_db_add_config_value('oVirtISOsRepositoryPath','/usr/share/ovirt-node-iso','general');
Line 340: select
fn_db_add_config_value('oVirtUpgradeScriptName','/usr/share/vdsm-reg/vdsm-upgrade','general');
Line 341: select
fn_db_add_config_value('oVirtUploadPath','/data/updates/ovirt-node-image.iso','general');
Line 342: select
fn_db_add_config_value('OvfUpdateIntervalInMinutes','1','general');
Line 343: select
fn_db_add_config_value('OvfItemsCountPerUpdate','100','general');
s/Count//, IMHO
Line 344: select fn_db_add_config_value('PayloadSize','8192','general');
Line 345: select fn_db_add_config_value('PosixStorageEnabled','false','2.2');
Line 346: select fn_db_add_config_value('PosixStorageEnabled','false','3.0');
Line 347: select fn_db_add_config_value('PosixStorageEnabled','true','3.1');
....................................................
File backend/manager/dbscripts/vms_sp.sql
Line 1: ----------------------------------------------------------------
Line 2: -- [vm_ovf_generations] Table
Line 3:
Line 4: Create or replace FUNCTION UpdateOvfGenerations(v_vms_ids
VARCHAR(5000), v_vms_db_generations VARCHAR(5000))
consider text instead of varchar(5000)
Line 5: RETURNS VOID
Line 6: AS $procedure$
Line 7: DECLARE
Line 8: curs_vmids CURSOR FOR SELECT ID from fnSplitterUuid(v_vms_ids);
Line 35: AS $procedure$
Line 36: BEGIN
Line 37: RETURN QUERY SELECT ovf.vm_guid as vm_guid
Line 38: FROM vm_ovf_generations ovf
Line 39: WHERE NOT EXISTS (SELECT 1 FROM vm_static v WHERE v.vm_guid =
ovf.vm_guid) AND
Use IN instead of EXISTS, it should perform better.
Line 40: ovf.storage_pool_id = v_storage_pool_id;
Line 41: END; $procedure$
Line 42: LANGUAGE plpgsql;
Line 43:
Line 67: AS $procedure$
Line 68: BEGIN
Line 69: RETURN QUERY SELECT templates.vmt_guid as vm_guid
Line 70: FROM vm_templates_view templates, vm_ovf_generations generations
Line 71: WHERE generations.vm_guid = templates.vmt_guid AND
templates.db_generation > generations.ovf_generation
break the AND up to its own line
Line 72: AND templates.storage_pool_id = v_storage_pool_id;
Line 73: END; $procedure$
Line 74: LANGUAGE plpgsql;
Line 75:
Line 84: AS $procedure$
Line 85: BEGIN
Line 86: RETURN QUERY SELECT vm.vm_guid as vm_guid
Line 87: FROM vms vm, vm_ovf_generations ovf_gen
Line 88: WHERE vm.vm_guid = ovf_gen.vm_guid AND
I like to have ANDs at the begging of the line, but I guess its a matter of
taste
Line 89: vm.db_generation > ovf_gen.ovf_generation AND
Line 90: vm.storage_pool_id = v_storage_pool_id;
Line 91: END; $procedure$
Line 92: LANGUAGE plpgsql;
Line 424: AS $procedure$
Line 425: BEGIN
Line 426: INSERT INTO vm_static(description, mem_size_mb, os, vds_group_id,
vm_guid, VM_NAME,
vmt_guid,domain,creation_date,num_of_monitors,allow_console_reconnect,is_initialized,is_auto_suspend,num_of_sockets,cpu_per_socket,usb_policy,
time_zone,auto_startup,is_stateless,dedicated_vm_for_vds, fail_back,
default_boot_sequence, vm_type, nice_level, default_display_type,
priority,iso_path,origin,initrd_url,kernel_url,kernel_params,migration_support,predefined_properties,userdefined_properties,min_allocated_mem,
entity_type, quota_id, cpu_pinning, is_smartcard_enabled)
Line 427: VALUES(v_description, v_mem_size_mb, v_os, v_vds_group_id,
v_vm_guid, v_vm_name, v_vmt_guid, v_domain, v_creation_date, v_num_of_monitors,
v_allow_console_reconnect, v_is_initialized, v_is_auto_suspend,
v_num_of_sockets, v_cpu_per_socket, v_usb_policy, v_time_zone,
v_auto_startup,v_is_stateless,v_dedicated_vm_for_vds,v_fail_back,
v_default_boot_sequence, v_vm_type, v_nice_level, v_default_display_type,
v_priority,v_iso_path,v_origin,v_initrd_url,v_kernel_url,v_kernel_params,v_migration_support,v_predefined_properties,v_userdefined_properties,v_min_allocated_mem,
'VM', v_quota_id, v_cpu_pinning, v_is_smartcard_enabled);
Line 428: INSERT INTO vm_ovf_generations(vm_guid, storage_pool_id) VALUES
(v_vm_guid, (select storage_pool_id from vds_groups vg where vg.vds_group_id =
v_vds_group_id));
remove TWS
Line 429: END; $procedure$
Line 430: LANGUAGE plpgsql;
Line 431:
Line 432:
Line 468: RETURNS VOID
Line 469: AS $procedure$
Line 470: DECLARE
Line 471: curs CURSOR FOR SELECT vms.vm_guid FROM
Line 472: vm_static vms WHERE vms.vds_group_id IN (SELECT v.vds_group_id
FROM vds_groups v, storage_pool sp WHERE sp.id=v_storage_pool_id AND
v.storage_pool_id = sp.id)
Remove TWS and fix indentation issues.
Line 473: ORDER BY vm_guid;
Line 474: id UUID;
Line 475: BEGIN
Line 476: OPEN curs;
Line 475: BEGIN
Line 476: OPEN curs;
Line 477: LOOP
Line 478: FETCH curs INTO id;
Line 479: IF NOT FOUND THEN
Use spaces, not tabs
Line 480: EXIT;
Line 481: END IF;
Line 482: UPDATE vm_static SET db_generation = db_generation + 1 WHERE
vm_guid = id;
Line 483: END LOOP;
Line 492: Create or replace FUNCTION
IncrementDbGenerationForAllInStoragePool(v_storage_pool_id UUID)
Line 493: RETURNS VOID
Line 494: AS $procedure$
Line 495: BEGIN
Line 496: UPDATE vm_static
Remove TWS
Line 497: SET db_generation = db_generation + 1
Line 498: WHERE storage_pool_id IN (SELECT v.vds_group_id FROM vds_groups
v, storage_pool sp where sp.id=v_storage_pool_id);
Line 499: END; $procedure$
Line 500: LANGUAGE plpgsql;
Line 727:
Line 728: Create or replace FUNCTION GetVmsByIds(v_vms_ids VARCHAR(5000))
RETURNS SETOF vms
Line 729: AS $procedure$
Line 730: BEGIN
Line 731: RETURN QUERY SELECT vm.*
Remove TWS
Line 732: FROM vms vm
Line 733: WHERE vm.vm_guid in (SELECT ID from fnSplitterUuid(v_vms_ids));
Line 734: END; $procedure$
Line 735: LANGUAGE plpgsql;
Line 729: AS $procedure$
Line 730: BEGIN
Line 731: RETURN QUERY SELECT vm.*
Line 732: FROM vms vm
Line 733: WHERE vm.vm_guid in (SELECT ID from fnSplitterUuid(v_vms_ids));
fix indentation
Line 734: END; $procedure$
Line 735: LANGUAGE plpgsql;
Line 736:
Line 737:
Line 877:
Line 878: INSERT INTO vm_dynamic(vm_guid, status) VALUES(v_vm_guid, 0);
Line 879:
Line 880: INSERT INTO vm_statistics(vm_guid) VALUES(v_vm_guid);
Line 881:
Remove TWS
Line 882: INSERT INTO vm_ovf_generations(vm_guid, storage_pool_id) VALUES
(v_vm_guid, (select storage_pool_id from vds_groups vg where vg.vds_group_id =
v_vds_group_id));
Line 883:
Line 884: UPDATE vm_static
Line 885: SET child_count =(SELECT COUNT(*) FROM vm_static WHERE vmt_guid
= v_vmt_guid)
....................................................
File backend/manager/dbscripts/vm_templates_sp.sql
Line 246:
Line 247:
Line 248:
Line 249:
Line 250: Create or replace FUNCTION GetVmTemplatesByIds(v_vms_tremplates_ids
VARCHAR(5000)) RETURNS SETOF vm_templates_view
consider text instead of varchar(5000)
Line 251: AS $procedure$
Line 252: BEGIN
Line 253: RETURN QUERY SELECT vm_templates.*
Line 254: FROM vm_templates_view vm_templates
Line 252: BEGIN
Line 253: RETURN QUERY SELECT vm_templates.*
Line 254: FROM vm_templates_view vm_templates
Line 255: WHERE vm_templates.vmt_guid in (SELECT ID from
fnSplitterUuid(v_vms_tremplates_ids));
Line 256: END; $procedure$
do you really need the internal select here?
Line 257: LANGUAGE plpgsql;
Line 258:
Line 259:
Line 260:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CommandBase.java
Line 248: return DbFacade.getInstance().getBusinessEntitySnapshotDao();
Line 249: }
Line 250:
Line 251:
Line 252: protected VmAndTemplatesGenerationsDAO
getVmAndTemplatesGenerationsDAO() {
Isn't command base a bit "low" for this?
Line 253: return
DbFacade.getInstance().getVmAndTemplatesGenerationsDAO();
Line 254: }
Line 255:
Line 256: protected VdsSpmIdMapDAO getVdsSpmIdMapDAO() {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OvfDataUpdater.java
Line 1: package org.ovirt.engine.core.bll;
General: javadoc all the methods. It's a bit hard to review like this.
General: Try to unify the methods for VMs and templates. There's a lot of
repetitive code there, that could perhaps be avoided with a general method and
different handlers/visitors passed. to it.
Line 2:
Line 3: import java.util.ArrayList;
Line 4: import java.util.HashMap;
Line 5: import java.util.LinkedList;
Line 45:
Line 46: public final class OvfDataUpdater {
Line 47: private static final Log log =
LogFactory.getLog(OvfDataUpdater.class);
Line 48: private static final OvfDataUpdater INSTANCE = new
OvfDataUpdater();
Line 49: private static final int ITEMS_COUNT_PER_UPDATE = Config.<Integer>
GetValue(ConfigValues.OvfItemsCountPerUpdate);
Don't save the ConfigValue to a constant - it prevents it from being reloadable.
Line 50: private static final int MAX_ITEMS_PER_SQL_STATEMENT = 100;
Line 51:
Line 52: private List<Guid> proccessedIdsInfo;
Line 53: private List<Long> proccessedOvfGenerationsInfo;
Line 46: public final class OvfDataUpdater {
Line 47: private static final Log log =
LogFactory.getLog(OvfDataUpdater.class);
Line 48: private static final OvfDataUpdater INSTANCE = new
OvfDataUpdater();
Line 49: private static final int ITEMS_COUNT_PER_UPDATE = Config.<Integer>
GetValue(ConfigValues.OvfItemsCountPerUpdate);
Line 50: private static final int MAX_ITEMS_PER_SQL_STATEMENT = 100;
Should be a ConfigValue, not a constant.
Line 51:
Line 52: private List<Guid> proccessedIdsInfo;
Line 53: private List<Long> proccessedOvfGenerationsInfo;
Line 54:
Line 49: private static final int ITEMS_COUNT_PER_UPDATE = Config.<Integer>
GetValue(ConfigValues.OvfItemsCountPerUpdate);
Line 50: private static final int MAX_ITEMS_PER_SQL_STATEMENT = 100;
Line 51:
Line 52: private List<Guid> proccessedIdsInfo;
Line 53: private List<Long> proccessedOvfGenerationsInfo;
First, I'm not sure if these need to be members.
In any event - I'm not a fan of these two lists.
How about a list of Pair<Guid, Long>, or some internal class?
Line 54:
Line 55: OvfManager ovfManager;
Line 56:
Line 57: private OvfDataUpdater(){
Line 51:
Line 52: private List<Guid> proccessedIdsInfo;
Line 53: private List<Long> proccessedOvfGenerationsInfo;
Line 54:
Line 55: OvfManager ovfManager;
private
Line 56:
Line 57: private OvfDataUpdater(){
Line 58: ovfManager = new OvfManager();
Line 59: proccessedIdsInfo = new LinkedList<Guid>();
Line 53: private List<Long> proccessedOvfGenerationsInfo;
Line 54:
Line 55: OvfManager ovfManager;
Line 56:
Line 57: private OvfDataUpdater(){
missing space between () and {
Line 58: ovfManager = new OvfManager();
Line 59: proccessedIdsInfo = new LinkedList<Guid>();
Line 60: proccessedOvfGenerationsInfo = new LinkedList<Long>();
Line 61: }
Line 78:
Line 79: @OnTimerMethodAnnotation("ovfUpdate_timer")
Line 80: public void ovfUpdate_timer() {
Line 81: log.info("OvfDataUpdater: Attempting to update VMs/Templates
Ovf.");
Line 82: List<storage_pool> storagePools = getStoragePoolDao().getAll();
I'd add a "getAllWithStatus(StoragePoolSatus)" method to the DAO.
Much more efficient than iterating over all of them.
Line 83: for (storage_pool pool : storagePools) {
Line 84: try {
Line 85: if (StoragePoolStatus.Up == pool.getstatus()) {
Line 86: clearProccessedInfo();
Line 83: for (storage_pool pool : storagePools) {
Line 84: try {
Line 85: if (StoragePoolStatus.Up == pool.getstatus()) {
Line 86: clearProccessedInfo();
Line 87: log.infoFormat("OvfDataUpdater: Attempting to
update vms Ovf in Data Center {0}",
s/vms Ovf/VM OVFs/
Line 88: pool.getname());
Line 89: updateOvfForVmsOfStoragePool(pool.getId());
Line 90:
Line 91: clearProccessedInfo();
Line 88: pool.getname());
Line 89: updateOvfForVmsOfStoragePool(pool.getId());
Line 90:
Line 91: clearProccessedInfo();
Line 92: log.infoFormat("OvfDataUpdater: Attempting to
update templates Ovf in Data Center {0}",
s/templates Ovf/VM template OVFs/
Line 93: pool.getname());
Line 94: updateOvfForTemplatesOfStoragePool(pool.getId());
Line 95:
Line 96: log.infoFormat("OvfDataUpdater: Attempting to
remove uneeded Ovfs in Data Center {0}",
Line 95:
Line 96: log.infoFormat("OvfDataUpdater: Attempting to
remove uneeded Ovfs in Data Center {0}",
Line 97: pool.getname());
Line 98: // remove deleted vm/templates ovf
Line 99:
removeOvfForTemplatesAndVmsOfStoragePool(pool.getId());
Add a log.info call beforehand.
Line 100: }
Line 101: } catch (Exception ex) {
Line 102: addAuditLogError(pool.getname());
Line 103: log.errorFormat("Exception while trying to update
VMs/Templates ovf in Data Center {0}, the exception is {1}",
Line 128: }
Line 129: }
Line 130: }
Line 131:
Line 132: private void clearProccessedInfo() {
Either put this later in the class or earlier - right now it's floating in the
middle.
Line 133: proccessedIdsInfo.clear();
Line 134: proccessedOvfGenerationsInfo.clear();
Line 135: }
Line 136:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBase.java
Line 109: @Column(name = "is_stateless")
Line 110: private boolean stateless;
Line 111:
Line 112: @Column(name = "db_generation")
Line 113: private Long db_generation;
java conventions: dbGeneration.
Line 114:
Line 115: @Column(name = "is_smartcard_enabled")
Line 116: private boolean smartcardEnabled;
Line 117:
Line 224: this.smartcardEnabled = smartcardEnabled;
Line 225: setQuotaId(quotaId);
Line 226: }
Line 227:
Line 228: public long getDb_generation() {
use java conventions.
Line 229: return db_generation;
Line 230: }
Line 231:
Line 232: public void setDb_generation(long db_generation) {
Line 228: public long getDb_generation() {
Line 229: return db_generation;
Line 230: }
Line 231:
Line 232: public void setDb_generation(long db_generation) {
use java conventions.
Line 233: this.db_generation = db_generation;
Line 234: }
Line 235:
Line 236: public List<VmNetworkInterface> getInterfaces() {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VM.java
Line 938: public void setnice_level(int value) {
Line 939: this.mVmStatic.setnice_level(value);
Line 940: }
Line 941:
Line 942: public void setDb_Generation(long value) {
Let's stick to java conventions: setDbGeneration and getDbGeneration.
Line 943: this.mVmStatic.setDb_generation(value);
Line 944: }
Line 945:
Line 946: public void getDb_Generation(long value) {
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/ConfigurationValues.java
Line 63: VdsFenceOptionTypes,
Line 64: ServerCPUList,
Line 65: SupportedClusterLevels(ConfigAuthType.User),
Line 66: OvfUpdateIntervalInMinutes,
Line 67: OvfItemsCountPerUpdate,
Why is this required?
The end-user (as opposed to the administrator) should be oblivious to this.
Line 68: ProductRPMVersion(ConfigAuthType.User),
Line 69: RhevhLocalFSPath,
Line 70: CustomPublicConfig_AppsWebSite(ConfigAuthType.User),
Line 71: DocsURL(ConfigAuthType.User),
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmAndTemplatesGenerationsDAO.java
Line 3: import java.util.List;
Line 4:
Line 5: import org.ovirt.engine.core.compat.Guid;
Line 6:
Line 7: public interface VmAndTemplatesGenerationsDAO extends DAO{
Consider OvfGenerationDao.
Also, consider extends GenericDao.
Line 8:
Line 9: /**
Line 10: * Updates the vms/templates ovf update version to the given value
Line 11: *
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmAndTemplatesGenerationsDbFacadeImpl.java
Line 7: import org.apache.commons.lang.StringUtils;
Line 8: import org.ovirt.engine.core.compat.Guid;
Line 9: import org.springframework.jdbc.core.simple.ParameterizedRowMapper;
Line 10:
Line 11: public class VmAndTemplatesGenerationsDbFacadeImpl extends
BaseDAODbFacade implements VmAndTemplatesGenerationsDAO {
If your interface extends GenericDao, you can implement
DefaultGenericDaoDbFacade, and get a bunch of code for free.
Line 12: @Override
Line 13: public void updateOvfGenerations(List<Guid> ids, List<Long>
values) {
Line 14: getCallsHandler().executeModification("UpdateOvfGenerations",
getCustomMapSqlParameterSource()
Line 15: .addValue("vms_ids", StringUtils.join(ids, ','))
Line 18:
Line 19: @Override
Line 20: public Long getOvfGeneration(Guid id) {
Line 21: return getCallsHandler().executeRead("GetOvfGeneration",
Line 22: new ParameterizedRowMapper<Long>() {
Add a createLongMapper() function to BaseDAODbFacade instead.
Line 23: @Override
Line 24: public Long mapRow(ResultSet rs, int rowNum)
throws SQLException {
Line 25: return rs.getLong(1);
Line 26: };
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDAO.java
Line 165: * @param quotaId
Line 166: * @param vmCount
Line 167: * @return
Line 168: */
Line 169: public List<VM> getVmsByIds(List<Guid> vmsIds);
The method name and the javadoc don't match
Line 170:
Line 171: /**
Line 172: * Retrieves the list of all VMS with optional permission
filtering.
Line 173: *
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmStaticDAODbFacadeImpl.java
Line 159:
Line 160: @Override
Line 161: public Long getDbGeneration(Guid id) {
Line 162: return getCallsHandler().executeRead("GetDbGeneration",
Line 163: new ParameterizedRowMapper<Long>() {
Here too - move this to createLondMapper in BaseDAODbFacade.
Line 164: @Override
Line 165: public Long mapRow(ResultSet rs, int rowNum)
throws SQLException {
Line 166: return rs.getLong(1);
Line 167: };
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmTemplateDAODbFacadeImpl.java
Line 91: return
getCallsHandler().executeReadList("GetAllVmTemplatesRelatedToQuotaId",
Line 92: VMTemplateRowMapper.instance,
Line 93: getCustomMapSqlParameterSource()
Line 94: .addValue("quota_id", quotaId));
Line 95:
why is this part of this patch?
Line 96: }
Line 97:
Line 98: @Override
Line 99: public Map<Boolean, VmTemplate> getAllForImage(Guid imageId) {
Line 191: getCustomMapSqlParameterSource()
Line 192: .addValue("network_id", id));
Line 193: }
Line 194:
Line 195:
why is this part of this patch?
Line 196: private final static class VMTemplateRowMapper extends
AbstractVmRowMapper<VmTemplate> {
Line 197: public static final VMTemplateRowMapper instance = new
VMTemplateRowMapper();
Line 198:
Line 199: @Override
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmTemplateDAO.java
Line 83: * @param quotaId
Line 84: * @param teamplatesCount
Line 85: * @return
Line 86: */
Line 87: public List<VmTemplate> getVmTemplatesByIds(List<Guid>
templatesIds);
The method name and the javadoc don't match.
Line 88:
Line 89: /**
Line 90: * Retrieves templates with permissions to perform the given
action.
Line 91: *
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfWriter.java
Line 235: }
Line 236:
Line 237: _writer.WriteStartElement("Generation");
Line 238: _writer.WriteRaw(String.valueOf(vmBase.getDb_generation()));
Line 239: _writer.WriteEndElement();
Shouldn't there be a symmetric call in OvfReader?
Line 240:
Line 241: _writer.WriteStartElement("VmType");
Line 242:
_writer.WriteRaw(String.valueOf(vmBase.getvm_type().getValue()));
Line 243: _writer.WriteEndElement();
....................................................
Commit Message
Line 4: Commit: Liron Aravot <[email protected]>
Line 5: CommitDate: 2012-12-02 13:27:57 +0200
Line 6:
Line 7: core: introducing OvfAutoUpdate
Line 8:
throughout this comment: s/vm/VM and s/ovf/OVF/
Line 9: vm/template configurations (including disks info) are stored on the
Line 10: master storage
Line 11: domain for backup purposes, import/export and also to provide the
Line 12: abillity to run VMs without having a running engine/db. Currently ovf
Line 6:
Line 7: core: introducing OvfAutoUpdate
Line 8:
Line 9: vm/template configurations (including disks info) are stored on the
Line 10: master storage
remove the newline here.
Line 11: domain for backup purposes, import/export and also to provide the
Line 12: abillity to run VMs without having a running engine/db. Currently ovf
Line 13: update is done synchronously when performing various operations on
Line 14: vms/templates - update, save, adding/removing a disk, etc. What's more,
Line 7: core: introducing OvfAutoUpdate
Line 8:
Line 9: vm/template configurations (including disks info) are stored on the
Line 10: master storage
Line 11: domain for backup purposes, import/export and also to provide the
what does import/export have to do with the master storage domain?
Line 12: abillity to run VMs without having a running engine/db. Currently ovf
Line 13: update is done synchronously when performing various operations on
Line 14: vms/templates - update, save, adding/removing a disk, etc. What's more,
Line 15: currently updating the ovf (updateVM vdsm call) is usually done within
a
Line 10: master storage
Line 11: domain for backup purposes, import/export and also to provide the
Line 12: abillity to run VMs without having a running engine/db. Currently ovf
Line 13: update is done synchronously when performing various operations on
Line 14: vms/templates - update, save, adding/removing a disk, etc. What's more,
s/What's more/moreover/
Line 15: currently updating the ovf (updateVM vdsm call) is usually done within
a
Line 16: transcation.
Line 17:
Line 18: The idea behined OvfAutoUpdater is to perform a batch ovf update
Line 14: vms/templates - update, save, adding/removing a disk, etc. What's more,
Line 15: currently updating the ovf (updateVM vdsm call) is usually done within
a
Line 16: transcation.
Line 17:
Line 18: The idea behined OvfAutoUpdater is to perform a batch ovf update
s/batch/batch of/
Line 19: operations that aggregate all hanging updates per data center. These
Line 20: updates will be done in specifed time intervals which will reduce
Line 21: XML-RPC calls and will enable the removal of this syncronous vdsm call
Line 22: from all over the code.
Line 15: currently updating the ovf (updateVM vdsm call) is usually done within
a
Line 16: transcation.
Line 17:
Line 18: The idea behined OvfAutoUpdater is to perform a batch ovf update
Line 19: operations that aggregate all hanging updates per data center. These
s/hanging/pending/ ?
Line 20: updates will be done in specifed time intervals which will reduce
Line 21: XML-RPC calls and will enable the removal of this syncronous vdsm call
Line 22: from all over the code.
Line 23:
Line 19: operations that aggregate all hanging updates per data center. These
Line 20: updates will be done in specifed time intervals which will reduce
Line 21: XML-RPC calls and will enable the removal of this syncronous vdsm call
Line 22: from all over the code.
Line 23:
What exactly is this list?
Line 24: *vdsm operation for removal of multiple vms/templates ovfs at once
need to be
Line 25: implemented
Line 26:
Line 27: * loadVm/VmTemplateData methods need to be revisited, if clauses in it
Line 27: * loadVm/VmTemplateData methods need to be revisited, if clauses in it
Line 28: might be irrelevant
Line 29:
Line 30: *UpdateVMVdsCommand error should be catched.
Line 31:
A link to feature page in oVirt wiki would be great.
Line 32: Change-Id: I9b5132300fb1f1fd94f771cab15efe5246dbeca8
--
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: 14
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Arik Hadas <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Liron Aravot <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: Vered Volansky <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
Gerrit-Reviewer: liron aravot <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
