Ori Liel has posted comments on this change. Change subject: restapi: Mappers for Gluster entities ......................................................................
Patch Set 11: (7 inline comments) .................................................... File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterOptionMapper.java Line 39: } Since we decided that options will not be business-entity, it's more consistent to have their mapping inside the entity which they belong to, meaning I'd move this code into VolumeMapper and get rid of this class. .................................................... File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterVolumeMapper.java Line 62: } "I can remove the bricks mapping in this case (BLL-to-REST). However in that case the output of GET request will look inconsistent from the representation accepted in the POST request (which contains bricks). What would you suggest?" --posting a volume with bricks inline is not REST-ful, but we enable it in this case since you said there's no concept of an empty volume. But we don't want to stray from REST principles any more than we have to. If the mapping of bricks is included here, it will hold also for update - and we don't want the user to update blocks within a volume by updating the volume. So if the mapping logic must stay, I believe it should be moved to BackendVolumesResource-->add(), and not be here in the mapper. And as for the direction Backend-->REST, meaning when displaying a volume, we would want to display only the volume-level data, and provide a link to the brick sub-collection, *even though* that is inconsistent with the fact that a user may, potentially, create a volume with bricks in it. So at the end of the day, I still believe the mapping of bricks should not be here. Line 130: mapping of bricks should be removed .................................................... File backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/GlusterOptionMapperTest.java Line 21: } If you remove OptionMapper class and move it's logic into VolumeMapper, move this testing logic into VolumeMapperTest accordingly .................................................... File backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/GlusterVolumeMapperTest.java Line 64: } assuming brick mapping logic is removed from VolumeMapper, remove brick mapping verification form the test Line 92: bye-bye bricks :) Line 97: } bye-bye bricks :) -- To view, visit http://gerrit.ovirt.org/3385 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I364d853106eda1a956241eb7210ce85781ee88b9 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shireesh Anjal <san...@redhat.com> Gerrit-Reviewer: Eoghan Glynn <eoghan.gl...@gmail.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com> Gerrit-Reviewer: Ori Liel <ol...@redhat.com> Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com> Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com> _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
