Tal Nisan has posted comments on this change. Change subject: core: Added a possibility to filter disks according explicitely set values ......................................................................
Patch Set 9: (3 comments) Some comments mostly minor, please refer to them and it's a +2 http://gerrit.ovirt.org/#/c/36060/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseGetAttachableDisksQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/BaseGetAttachableDisksQuery.java: Line 4: import org.ovirt.engine.core.common.businessentities.Disk; Line 5: import org.ovirt.engine.core.common.queries.GetAllAttachableDisks; Line 6: import org.ovirt.engine.core.compat.Version; Line 7: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 8: import org.springframework.util.CollectionUtils; Same implementation of isEmpty(), but we mostly use org.apache.commons.collections.CollectionUtils, I'd move to use that Line 9: Line 10: import java.util.ArrayList; Line 11: import java.util.List; Line 12: Line 24: getParameters().getVmId(), Line 25: getUserID(), Line 26: getParameters().isFiltered()); Line 27: if (CollectionUtils.isEmpty(diskList)) { Line 28: setReturnValue(new ArrayList<>()); Why can't you just return diskList? If I'm not mistaken DbFacade does not return null lists, only empty Line 29: return; Line 30: } Line 31: Line 32: setReturnValue(filterDisks(diskList)); http://gerrit.ovirt.org/#/c/36060/9/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllAttachableDisksQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetAllAttachableDisksQuery.java: Line 7: import org.ovirt.engine.core.common.businessentities.VM; Line 8: import org.ovirt.engine.core.common.queries.GetAllAttachableDisks; Line 9: import org.ovirt.engine.core.dal.dbbroker.DbFacade; Line 10: Line 11: public class GetAllAttachableDisksQuery<P extends GetAllAttachableDisks> extends BaseGetAttachableDisksQuery<P> { Given the nature of this query, isn't it better to call it GetAllAttachableDisksForVmQuery? Makes it easier to understand the purpose of the query, especially now when we will have more than one query to attachable disks Line 12: Line 13: public GetAllAttachableDisksQuery(P parameters) { Line 14: super(parameters); Line 15: } -- To view, visit http://gerrit.ovirt.org/36060 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If363cd92aa0071c88f2333294002ff8aa0da1b40 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Ala Hino <ah...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Candace Sheremeta <csher...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Freddy Rolland <froll...@redhat.com> Gerrit-Reviewer: Idan Shaby <ish...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vered Volansky <vvola...@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
