Allon Mureinik has posted comments on this change.
Change subject: core: add repo_file_id to repo_file_meta_data
......................................................................
Patch Set 2: I would prefer that you didn't submit this
(9 inline comments)
....................................................
File backend/manager/dbscripts/create_views.sql
Line 90: storage_domain_static.id as storage_domain_id,
Line 91: storage_domain_static.storage_domain_type as
storage_domain_type,
Line 92: storage_pool_iso_map.storage_pool_id as storage_pool_id,
Line 93: storage_pool_iso_map.status as storage_domain_status,
Line 94: repo_file_meta_data.repo_image_id as repo_image_id,
what about repo_image_name?
Line 95: repo_file_meta_data.size as size,
Line 96: repo_file_meta_data.date_created as date_created,
Line 97: repo_file_meta_data.last_refreshed as last_refreshed,
Line 98: repo_file_meta_data.file_type as file_type,
....................................................
File backend/manager/dbscripts/upgrade/03_03_0040_repo_file_id.sql
Line 1: -- The current file names from now on will be used as images id in the
ISO domains
Line 2: ALTER TABLE repo_file_meta_data RENAME COLUMN repo_file_name TO
repo_image_id;
please use fn_db_rename_column
Line 1: -- The current file names from now on will be used as images id in the
ISO domains
Line 2: ALTER TABLE repo_file_meta_data RENAME COLUMN repo_file_name TO
repo_image_id;
Line 3: ALTER TABLE repo_file_meta_data ADD COLUMN repo_image_name VARCHAR(256);
please use fn_db_add_column
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IsoDomainListSyncronizer.java
Line 515: repo_md.setSize(0);
Line 516:
repo_md.setRepoDomainId(repoStorageDomainId);
Line 517: repo_md.setDateCreated(null);
Line 518: repo_md.setRepoImageId(isoFile);
Line 519: repo_md.setRepoImageName(null);
why don't we have an imageName?
Line 520: repo_md.setFileType(imageType);
Line 521:
repoFileMetaDataDao.addRepoFileMap(repo_md);
Line 522: }
Line 523: return true;
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmCommand.java
Line 606: ImageFileType.ISO);
Line 607: Version bestClusterVer = null;
Line 608: int bestToolVer = 0;
Line 609: for (RepoFileMetaData map : repoFilesMap) {
Line 610: String fileName = map.getRepoImageId() != null ?
map.getRepoImageId() : "";
consider replacing this ugly call with
StringUtils.defaultString(map.getRepoImageId(), "") while you're at it
Line 611: Matcher matchToolPattern =
Line 612:
Pattern.compile(IsoDomainListSyncronizer.getRegexToolPattern()).matcher(fileName);
Line 613: if (matchToolPattern.find()) {
Line 614: // Get cluster version and tool version of Iso
tool.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/RepoFileMetaData.java
Line 140: if (repoImageName != null) {
Line 141: /* To provide an hint about the image id and at the same
time
Line 142: * maintain the image title short we just report 7
characters
Line 143: * of the id (similarly to what git does with hashes).
Line 144: */
I'd use // comments instead of /* */, but that's just me `-)
Line 145: return repoImageName + " (" + repoImageId.substring(0, 7)
+ ")";
Line 146: } else {
Line 147: return repoImageId;
Line 148: }
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/RepoFileMetaDataDAODbFacadeImpl.java
Line 116: entity.setLastRefreshed(rs.getLong("last_refreshed"));
Line 117:
entity.setFileType(ImageFileType.forValue(rs.getInt("file_type")));
Line 118: return entity;
Line 119: }
Line 120: }
This seems like a very outdated revision of the file.
commit f26882ff introduced yet another RowMapper - it too must be change in the
same fashion
....................................................
File
backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/RepoFileMetaDataDAOTest.java
Line 207: assertSame(true, !listOfRepoFiles.isEmpty());
Line 208: assertSame(
Line 209: true,
Line 210: listOfRepoFiles.get(0).getRepoImageId()
Line 211: .equals(newRepoFileMap.getRepoImageId()));
This is the worst way I can imagine to implement an equality check.
It's unrelated to your patch, but I had to get it off my chest.
Line 212: assertSame(true,
Line 213: listOfRepoFiles.get(0).getLastRefreshed() ==
newRepoFileMap
Line 214: .getLastRefreshed());
Line 215: assertSame(true,
....................................................
File backend/manager/modules/dal/src/test/resources/fixtures.xml
Line 3873:
Line 3874: <table name="repo_file_meta_data">
Line 3875: <column>repo_domain_id</column>
Line 3876: <column>repo_image_id</column>
Line 3877: <column>size</column>
what about repo_image_name?
Line 3878: <column>last_refreshed</column>
Line 3879: <column>file_type</column>
Line 3880: <row>
Line 3881: <value>d034f3b2-fb9c-414a-b1be-1e642cfe57ae</value>
--
To view, visit http://gerrit.ovirt.org/13958
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I2791fa22820fc021938709f4a921ab4d6213fab7
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Itamar Heim <ih...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com>
Gerrit-Reviewer: Ravi Nori <rn...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
