Alon Bar-Lev has posted comments on this change.

Change subject: [WIP] oVirt Node Upgrade: Support N configurations
......................................................................


Patch Set 4: (5 inline comments)

....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 353: select 
fn_db_add_config_value('OvfVirtualSystemType','ENGINE','general');
Line 354: --Handling The ovirt-node installation files path
Line 355: select 
fn_db_add_config_value('OvirtInitialSupportedIsoVersion','5.8:2.5.5','general');
Line 356: select 
fn_db_add_config_value('OvirtIsoPrefix','rhev:ovirt-node','general');
Line 357: select 
fn_db_add_config_value('OvirtPattern','(ovirt-node|rhev)-(.*)\.iso','general');
single pattern per component please. we cannot assume the pattern can be shared.

Also, I thought we agreed to add the pattern to the prefix...
Line 358: select 
fn_db_add_config_value('oVirtISOsRepositoryPath','/usr/share/rhev-hypervisor:/usr/share/ovirt-node-iso','general');
Line 359: select 
fn_db_add_config_value('oVirtUpgradeScriptName','/usr/share/vdsm-reg/vdsm-upgrade','general');
Line 360: select 
fn_db_add_config_value('oVirtUploadPath','/data/updates/ovirt-node-image.iso','general');
Line 361: select 
fn_db_add_config_value('OvfUpdateIntervalInMinutes','60','general');


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetoVirtISOsQuery.java
Line 178:         }
Line 179:         return null;
Line 180:     }
Line 181: 
Line 182:     private List<String> getListOfIsoFiles(File directory, 
OVirtNodeInfo info) {
but you removed the static from there as well.
Line 183:         List<String> isoFileList = new ArrayList<String>();
Line 184:         File[] filterOvirtFiles = filterOvirtFiles(directory, 
getIsoPattern(info));
Line 185:         for (File file : filterOvirtFiles) {
Line 186:             isoFileList.add(file.getName());


Line 279:      * Returns the pattern for ISO files.
Line 280:      * Since the prefix from the configuration may change (reloadable 
configuration), it is checked each time.
Line 281:      * A cached version of pattern is saved, though, to avoid the 
overhead of re-compiling it.
Line 282:      */
Line 283:     private Pattern getIsoPattern(OVirtNodeInfo info) {
why have you removed the static?
Line 284:         log.infoFormat("getIsoPatter - pattern [{0}]", info.pattern);
Line 285:         String expectedPattern = info.pattern;
Line 286:         if (isoPattern == null || 
!expectedPattern.equals(isoPattern.toString())) {
Line 287:             isoPattern = Pattern.compile(expectedPattern);


....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OVirtNodeInfo.java
Line 22:   public static OVirtNodeInfo[] get() {
Line 23:       final String delimiter = ":";
Line 24:       final String prefix[] = Config.<String> 
GetValue(ConfigValues.OvirtIsoPrefix).split(delimiter);
Line 25:       final String path[] = 
Config.resolveOVirtISOsRepositoryPath().split(delimiter);
Line 26:       final String pattern = Config.<String> 
GetValue(ConfigValues.OvirtPattern);
pattern should follow the same principle, a pattern per configuration. it is 
important to be consistent.

also, please use the prefix as pattern.
Line 27:       final String minimumVersion[] = Config.<String> 
GetValue(ConfigValues.OvirtInitialSupportedIsoVersion).split(delimiter);
Line 28: 
Line 29:       if (prefix.length != minimumVersion.length ||
Line 30:               prefix.length != path.length) {


....................................................
File 
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1375:     MemorySnapshotSupported(511),
Line 1376: 
Line 1377:     @TypeConverterAttribute(String.class)
Line 1378:     @DefaultValueAttribute("(ovirt-node|rhev)-(.*)\\.iso")
Line 1379:     OvirtPattern(512),
No need for that, you can use the prefix as pattern.
Line 1380: 
Line 1381:     Invalid(65535);
Line 1382: 
Line 1383:     private int intValue;


-- 
To view, visit http://gerrit.ovirt.org/14756
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibfb9dc5d0dc8780b519107acbe0ae866831f782c
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com>
Gerrit-Reviewer: Barak Azulay <bazu...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Douglas Schilling Landgraf <dougsl...@redhat.com>
Gerrit-Reviewer: Michael Burns <mbu...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to