Omer Frenkel has posted comments on this change. Change subject: core: Vm Icons - backend part ......................................................................
Patch Set 14: (15 comments) partially reviewed, this patch is really too big some comments for now https://gerrit.ovirt.org/#/c/38600/14//COMMIT_MSG Commit Message: Line 5: CommitDate: 2015-04-16 14:38:44 +0200 Line 6: Line 7: core: Vm Icons - backend part Line 8: Line 9: http://www.ovirt.org/Features/VM_Icons did you update the wiki to reflect latest changes? Line 10: Line 11: Backand part of introduction of Vm Icons. It replaces current system of Line 12: providing operating system icons for browser and allows users to Line 13: customize icons. https://gerrit.ovirt.org/#/c/38600/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 1458: Line 1459: /** Line 1460: * Icon processing policy: Line 1461: * <ul> Line 1462: * <li>If there is an attached icon, it is used as large icon as as base for computation of small icon. typo 'as as' Line 1463: * Predefined icons should not be sent in parameters.</li> Line 1464: * <li>If there are no icon in parameters && both (small and large) icon ids are set then those ids are used. Line 1465: * </li> Line 1466: * <li>Otherwise (at least one icon id is null) both icon ids are coppied from template.</li> Line 1472: final VmIconIdSizePair iconIds = Line 1473: IconUtils.ensureIconPairInDatabase(getParameters().getVmLargeIcon()); Line 1474: vmStatic.setLargeIconId(iconIds.getLarge()); Line 1475: vmStatic.setSmallIconId(iconIds.getSmall()); Line 1476: return; this is not so nice, i think its better to use if..else-if instead Line 1477: } Line 1478: if (vmStatic.getLargeIconId() == null Line 1479: || vmStatic.getSmallIconId() == null) { Line 1480: vmStatic.setSmallIconId(getVmTemplate().getSmallIconId()); https://gerrit.ovirt.org/#/c/38600/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/Backend.java: Line 270: 1, quotaCacheIntervalInMinutes, TimeUnit.MINUTES); Line 271: //initializes attestation Line 272: initAttestation(); Line 273: updateIcons(); Line 274: iconCleanup(); can these operations be time consuming? mainly the updateIcons? need to make sure jboss will not time out the backend startup Line 275: EngineExtensionsManager.getInstance().engineInitialize(); Line 276: AuthenticationProfileRepository.getInstance(); Line 277: AcctUtils.reportReason(Acct.ReportReason.STARTUP, "Starting up engine"); Line 278: } https://gerrit.ovirt.org/#/c/38600/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmIconsQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmIconsQuery.java: Line 26: } Line 27: Line 28: @Override Line 29: protected void executeQueryCommand() { Line 30: switch (getParameters().getVerb()) { why having 2 queries in 1? Line 31: case GetIcons: Line 32: getIcons(); Line 33: break; Line 34: case GetOsDefaultIconsMap: https://gerrit.ovirt.org/#/c/38600/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IconLoader.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/IconLoader.java: Line 41: private static final Path ICONS_DIR = EngineLocalConfig.getInstance().getUsrDir().toPath().resolve("icons"); Line 42: private static final Path LARGE_ICON_DIR =ICONS_DIR.resolve("large"); Line 43: private static final Path SMALL_ICON_DIR =ICONS_DIR.resolve("small"); Line 44: private final Map<Integer, VmIconIdSizePair> osIdToIconIdMap = new HashMap<>(); Line 45: private final int DEFAULT_OS_ID = 0; please use OsRepository.DEFAULT_X86_OS instead Line 46: Line 47: private IconLoader() { Line 48: loadIconsToDatabase(); Line 49: ensureDefaultOsIconExists(); Line 53: Line 54: private void loadIconsToDatabase() { Line 55: final HashMap<Integer, String> osIdToOsNameMap = Line 56: SimpleDependecyInjector.getInstance().get(OsRepository.class).getUniqueOsNames(); Line 57: for (Map.Entry<Integer, String> entry : osIdToOsNameMap.entrySet()) { wouldn't it be more efficient to get all default icons from db and compare to the os list, and insert only those that are mission? because i see ensureIcons..() method looks complicated (read icon from disk, then try to insert to db, and the get back from db) Line 58: final VmIconIdSizePair iconIdPair = ensureIconsInDatabase(entry.getValue()); Line 59: if (iconIdPair != null) { Line 60: osIdToIconIdMap.put(entry.getKey(), iconIdPair); Line 61: } Line 90: } Line 91: return osIdToIconIdMap.get(DEFAULT_OS_ID); Line 92: } Line 93: Line 94: private void updateVmIconDefaultsTable() { what is going on here (missing doc)? it seems you are re-creating the defaults table, why? Line 95: DbFacade.getInstance().getVmIconsDefaultDao().removeAll(); Line 96: for (Map.Entry<Integer, VmIconIdSizePair> entry : osIdToIconIdMap.entrySet()) { Line 97: final VmIconDefault osDefautlIconIds = new VmIconDefault(Guid.newGuid(), Line 98: entry.getKey(), https://gerrit.ovirt.org/#/c/38600/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/IconUtils.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/utils/IconUtils.java: Line 76: throw new RuntimeException(e); Line 77: } Line 78: } Line 79: Line 80: public static String toDataUrl(byte[] data, IconValidator.FileType type) { please add java doc for public methods, its not clear what the methods do Line 81: // data:[<MIME-type>][;charset=<encoding>][;base64],<data> Line 82: final String base64Data = DatatypeConverter.printBase64Binary(data); Line 83: final String dataUrl = new StringBuilder() Line 84: .append("data:") Line 107: /** Line 108: * @return list of icon ids that was previously used and are not used any more by this vm_static entity Line 109: */ Line 110: public static List<Guid> updateVmIcon(VmBase originalVmBase, VmBase newVmBase, String vmIconParameter) { Line 111: if (vmIconParameter != null) { dont we want to check that its not the same icon? Line 112: addNewIconPair(newVmBase, vmIconParameter); Line 113: } else { Line 114: if (newVmBase.getSmallIconId() == null) { Line 115: computeSmallByLargeIconId(newVmBase); https://gerrit.ovirt.org/#/c/38600/14/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VmIconIdSizePair.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/VmIconIdSizePair.java: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: import java.io.Serializable; Line 6: Line 7: public class VmIconIdSizePair implements Serializable{ why not just use Pair<,> ? does this have any advantages? where is it used? (beside the result of the query) Line 8: Line 9: private Guid small; Line 10: private Guid large; Line 11: https://gerrit.ovirt.org/#/c/38600/14/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmIconDaoImpl.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmIconDaoImpl.java: Line 65: Line 66: @Override Line 67: public Guid ensureIconInDatabase(final String icon) { Line 68: if (icon == null) { Line 69: throw new IllegalArgumentException("Argument 'icon' should not be null"); //$NON-NLS-1$ no need for this gwt comment Line 70: } Line 71: try { Line 72: TransactionSupport.executeInNewTransaction(new TransactionMethod<Void>() { Line 73: @Override Line 79: }); Line 80: } catch (RuntimeException ex) { Line 81: // nothing, icon is already in db Line 82: } Line 83: final List<VmIcon> iconsOfRequiredData = getByDataUrl(icon); i think you can change this to first do get by data from db, and if doesn't exists add. put both in a transaction and this should be thread safe Line 84: if (iconsOfRequiredData.size() != 1) { Line 85: throw new VdcBLLException(VdcBllErrors.VM_ICON_NOT_FOUND); Line 86: } Line 87: return iconsOfRequiredData.get(0).getId(); https://gerrit.ovirt.org/#/c/38600/14/packaging/dbscripts/upgrade/03_06_1250_add_vm_icons_vm_icon_defaults_tables.sql File packaging/dbscripts/upgrade/03_06_1250_add_vm_icons_vm_icon_defaults_tables.sql: Line 1: CREATE TABLE vm_icons ( Line 2: id UUID NOT NULL, Line 3: data_url VARCHAR(32768) NOT NULL -- 1024 * 32 Line 4: ); Line 5: -- table viewer http://jsfiddle.net/jniederm/u1bf0767/ this didnt really work for me Line 6: Line 7: SELECT fn_db_create_constraint('vm_icons', Line 8: 'pk_vm_icons', Line 9: 'PRIMARY KEY (id)'); Line 8: 'pk_vm_icons', Line 9: 'PRIMARY KEY (id)'); Line 10: CREATE UNIQUE INDEX vm_icons_data_url_unique_index ON vm_icons( md5(data_url) ); Line 11: Line 12: CREATE TABLE vm_icon_defaults ( shouldn't this be icon_info? Line 13: id UUID NOT NULL, Line 14: os_id INTEGER NOT NULL, Line 15: small_icon_id UUID NOT NULL, Line 16: large_icon_id UUID NOT NULL -- To view, visit https://gerrit.ovirt.org/38600 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I98ad0d76285af3b2913e477a340bfd1ac09a296e Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Sandro Bonazzola <sbona...@redhat.com> Gerrit-Reviewer: Yedidyah Bar David <d...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
