Jakub Niedermertl has posted comments on this change. Change subject: core: Vm Icons - backend part ......................................................................
Patch Set 14: (15 comments) 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? now yes 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' Done 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 Done 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? updateIcons works only with predefined icons - a limited data set. There are currently about 30 operating systems times 2 icon sizes = at most about 60 icons to update. I can't even notice it on my machine. So I thing it is not a problem, but I'm open to different solutions. 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? I saw OsRepositoryQuery and thought it is how we deal with related tasks. It can be split to separate queries. 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 Partially done. I tried to avoid use of DEFAULT_X86_OS because I consider it misleading. I use DEFAULT_OS_ID to reference default os in general (for all architectures) not only x86. It is because there is one image (Other OS box) for all architectures. 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 I want not only insert missing icons but also update changed one. It is about balancing maintainability and speed. Current version is short, reuses code and IMHO less error prone. I can also rewrite it to read icons from database, update changed icons, insert missing ones. It would be probably faster but it works on small data set - there are about 30 entries in the map. Say what's better. 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)? doc added. Yes it recreates the vm_icon_defaults table because the table is expected to be small (now about 30 rows, one row per one predefined operating system) - so it is affortable and computing diff is far more complicated and error prone. 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 Done 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? I don't thing so. It is responsibility of frontend part to send null in vmIconParameter if there is no change. So scenario with the same icon should be really rare. 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<,> ? 1. This seems to me more expressive. 2. Utility getter Guid get(boolean small) simplifies code like: Guid id = small ? pair.getSmall() : part.getLarge(); 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 Done 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 Done 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 It works for me, both in psql and pgAdmin You can try: psql -U engine -d engine -c 'copy vm_icons to stdout; copy vm_icon_defaults to stdout;' | xclip -selection clipboard and then paste into the textarea. 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? The table holds defaults for icons, so I named it so. User-uploaded icons are not referenced from this table. If 'icon_info' seems more expressive to you, it can be definitely renamed. 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
