Sahina Bose has posted comments on this change. Change subject: engine : Bll Command for Create Geo Rep session ......................................................................
Patch Set 27: (9 comments) https://gerrit.ovirt.org/#/c/29834/27/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeGeoRepSessionCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeGeoRepSessionCommand.java: Line 49: Is there a need to override this? Can you not move the setVdsGroupId(..) and setGlusterVolumeId(..) call to the constructor? Line 62: Add check that georep is supported in cluster level Line 72: slaveVolume != null check not required as you would have returned from function. can you split into separate checks for readability? Line 83: return possible NPE, as you haven't checked if slaveHost is null or not in canDoAction Line 116: why not call this succeeded? :) https://gerrit.ovirt.org/#/c/29834/27/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/SetupGlusterGeoRepMountBrokerCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/SetupGlusterGeoRepMountBrokerCommand.java: Line 48: slaveVolume = Line 49: getSlaveVolume(); Line 50: if (slaveVolume != null) { Line 51: setGlusterVolumeId(slaveVolume.getId()); Line 52: setVdsGroupId(slaveVolume.getClusterId()); move to constructor? Line 53: } Line 54: if (slaveVolume == null) { Line 55: return false; Line 56: } https://gerrit.ovirt.org/#/c/29834/27/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeGeoRepSessionCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/CreateGlusterVolumeGeoRepSessionCommandTest.java: Line 73: you could move these common expectations to a setup method that you can call in every test https://gerrit.ovirt.org/#/c/29834/27/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/SetupGlusterGeoRepMountBrokerCommandTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/SetupGlusterGeoRepMountBrokerCommandTest.java: Line 40: volume.setName(""); Line 41: return volume; Line 42: } Line 43: Line 44: private VDS getVds(VDSStatus status) { You can mock VDS and GlusterVolumeEntity as well Line 45: VDS host = new VDS(); Line 46: host.setId(vdsGroupId); Line 47: host.setStatus(status); Line 48: host.setVdsGroupId(Guid.newGuid()); https://gerrit.ovirt.org/#/c/29834/27/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/GlusterVolumeGeoRepSessionParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/gluster/GlusterVolumeGeoRepSessionParameters.java: Line 26: this pass userName, as you have initialised it -- To view, visit https://gerrit.ovirt.org/29834 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iccc92235deea42d7818336b2402476193cbe789c Gerrit-PatchSet: 27 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
