Omer Frenkel has posted comments on this change. Change subject: core: Vm Icons - backend part ......................................................................
Patch Set 25: (9 comments) looks ok, some last comments :) https://gerrit.ovirt.org/#/c/38600/25/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 32: getIcons(); Line 33: break; Line 34: case GetOsDefaultIconsMap: Line 35: getOsDefaultIconsMap(); Line 36: break; please separate to 2 queries, there is no reason to have this here Line 37: } Line 38: } Line 39: Line 40: /** Line 37: } Line 38: } Line 39: Line 40: /** Line 41: * returned type: Map<Guid, String> requested icon id -> icon data "<" -> "<" Line 42: */ Line 43: private void getIcons() { Line 44: Map<Guid, String> result = new HashMap<>(); Line 45: for (Guid iconId : getParameters().getIconIds()) { Line 50: setReturnValue(result); Line 51: } Line 52: Line 53: /** Line 54: * returned type: Map<Integer, Guid> osId -> iconId also here Line 55: */ Line 56: public void getOsDefaultIconsMap() { Line 57: final Map<Integer, VmIconIdSizePair> result = new HashMap<>(); Line 58: final List<VmIconDefault> iconDefaults = vmIconDefaultDao.getAll(); https://gerrit.ovirt.org/#/c/38600/25/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmCommand.java: Line 77: import java.util.Date; Line 78: import java.util.HashMap; Line 79: import java.util.List; Line 80: import java.util.Map; Line 81: import java.util.Objects; please undo this change, its not related and wrong (java comes before org) Line 82: Line 83: public class UpdateVmCommand<T extends VmManagementParametersBase> extends VmManagementCommandBase<T> Line 84: implements QuotaVdsDependent, RenamedEntityInfoProvider{ Line 85: https://gerrit.ovirt.org/#/c/38600/25/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java: Line 51: private VmTemplate mOldTemplate; Line 52: private List<GraphicsDevice> cachedGraphics; Line 53: Line 54: @Inject Line 55: private VmIconDao vmIconDao; i think this is not in use and can be removed Line 56: Line 57: public UpdateVmTemplateCommand(T parameters) { Line 58: super(parameters); Line 59: setVmTemplate(parameters.getVmTemplateData()); https://gerrit.ovirt.org/#/c/38600/25/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmCommand.java: Line 520: return getDbFacade().getVmDeviceDao(); Line 521: } Line 522: Line 523: protected VmIconDao getVmIconDao() { Line 524: return vmIconDao; are these in use? its hard to see Line 525: } Line 526: Line 527: protected VmIconDefaultDao getVmIconDefaultDao() { Line 528: return vmIconDefaultDao; https://gerrit.ovirt.org/#/c/38600/25/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/IconValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/IconValidator.java: Line 19: import java.util.List; Line 20: import java.util.regex.Matcher; Line 21: import java.util.regex.Pattern; Line 22: Line 23: public class IconValidator { this class really need to be unit tested, can be in different patch Line 24: Line 25: private static final int MAX_DATAURL_SIZE = 32 * 1024; Line 26: private final DimensionsType dimensionsType; Line 27: private String dataUrl; https://gerrit.ovirt.org/#/c/38600/25/packaging/dbscripts/create_views.sql File packaging/dbscripts/create_views.sql: Line 1120: AND snapshot_type = 'NEXT_RUN' Line 1121: WHERE Line 1122: vm_static.entity_type = 'VM'; Line 1123: CREATE Line 1124: OR REPLACE VIEW vms_with_tags AS should add the fields here as well, i think Line 1125: SELECT Line 1126: vms.vm_name, Line 1127: vms.mem_size_mb, Line 1128: vms.nice_level, https://gerrit.ovirt.org/#/c/38600/25/packaging/dbscripts/vms_sp.sql File packaging/dbscripts/vms_sp.sql: Line 556: v_val UUID; Line 557: BEGIN Line 558: -- lock template for child count update Line 559: select vm_guid into v_val FROM vm_static WHERE vm_guid = v_vmt_guid for update; Line 560: INSERT INTO vm_static(description, please dont do refactoring with code changes, it is very hard to spot an error, especially here Line 561: free_text_comment, Line 562: mem_size_mb, Line 563: os, Line 564: vds_group_id, -- 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: 25 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
