Alon Bar-Lev has posted comments on this change.
Change subject: oVirt Node Upgrade: Support N configuration
......................................................................
Patch Set 6: (4 inline comments)
Hello Douglas,
The node info data structure should reflect the data structure with no magic.
All it need to hold are three fields:
String base;
String minimumVersion;
Pattern prefix;
There is no need to do any magic or regexp match at the get() method, just read
the values from database and return the data structure.
Instead of using the prefix in code you should list the directories under base
and accept only these which matches the pattern, extracting the version from $1.
Thanks!
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 352: select fn_db_add_config_value('OriginType','OVIRT','general');
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','2.5.5:5.8','general');
Line 356: select
fn_db_add_config_value('OvirtIsoPrefix','(ovirt-node)-(.*)\.iso:(rhev)-(.*)\.iso','general');
why the ovirt-node and rhev are within group? where is it needed?
please add ^ and $ so that this will be an exact match.
Line 357: select
fn_db_add_config_value('oVirtISOsRepositoryPath','/usr/share/ovirt-node-iso:/usr/share/rhev-hypervisor','general');
Line 358: select
fn_db_add_config_value('oVirtUpgradeScriptName','/usr/share/vdsm-reg/vdsm-upgrade','general');
Line 359: select
fn_db_add_config_value('oVirtUploadPath','/data/updates/ovirt-node-image.iso','general');
Line 360: select
fn_db_add_config_value('OvfUpdateIntervalInMinutes','60','general');
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/OVirtNodeInfo.java
Line 35: // Prefix Found: ovirt-node
Line 36: final String regex[] = Config.<String>
GetValue(ConfigValues.OvirtIsoPrefix).split(delimiter);
Line 37:
Line 38: // Setting pattern to get prefix from OvirtIsoPrefix
Line 39: Pattern exp = Pattern.compile("\\((.*?)\\)", Pattern.DOTALL);
why do you need this exp?
Line 40:
Line 41: String prefix = null;
Line 42: List<OVirtNodeInfo> info = new LinkedList<OVirtNodeInfo>();
Line 43:
Line 52: for (int i=0; i < regex.length; i++) {
Line 53: log.debugFormat("NodeInfo: regex {0} path {1},
minimumVersion {2}",
Line 54: regex[i], path[i], minimumVersion[i]);
Line 55:
Line 56: // Getting prefix from regex
I don't understand why you need to get a prefix if you have the regexp.
Line 57: Matcher matcher = exp.matcher(regex[i]);
Line 58: matcher.find();
Line 59: prefix = matcher.group(1);
Line 60: log.infoFormat("matcher group: [{0}] prefix found: [{1}]",
regex[i], prefix);
Line 58: matcher.find();
Line 59: prefix = matcher.group(1);
Line 60: log.infoFormat("matcher group: [{0}] prefix found: [{1}]",
regex[i], prefix);
Line 61:
Line 62: info.add(new OVirtNodeInfo(prefix, path[i],
minimumVersion[i], regex[i]));
instead of regex[1] please hold compiled pattern, Pattern.compile(regex[i])
Line 63: }
Line 64: return info.toArray(new OVirtNodeInfo[info.size()]);
Line 65: }
--
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: 6
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>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
