anmolbabu has posted comments on this change. Change subject: engine : Bll Command for Create Geo Rep session ......................................................................
Patch Set 27: (9 comments) Comments incorporated in latest patch set 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 45: } Line 46: Line 47: Line 48: @Override Line 49: public VDSGroup getVdsGroup() { > Is there a need to override this? Done Line 50: setVdsGroupId(getGlusterVolume().getClusterId()); Line 51: return super.getVdsGroup(); Line 52: } Line 53: Line 58: } Line 59: Line 60: @Override Line 61: protected boolean canDoAction() { Line 62: slaveVolume = > Add check that georep is supported in cluster level Done Line 63: getSlaveVolume(); Line 64: if (slaveVolume == null) { Line 65: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_INVALID); Line 66: } Line 68: GlusterGeoRepSession geoRepSession = Line 69: getGeoRepDao().getGeoRepSession(getGlusterVolumeId(), Line 70: getParameters().getSlaveHost(), Line 71: getParameters().getSlaveVolumeName()); Line 72: return super.canDoAction() && slaveVolume != null ? (geoRepSession == null ? (allRemoteServersUp ? true > slaveVolume != null check not required as you would have returned from func Done Line 73: : failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_ONE_OR_MORE_REMOTE_HOSTS_ARE_NOT_ACCESSIBLE)) Line 74: : failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_GEOREP_SESSION_ALREADY_CREATED)) Line 75: : failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_VOLUME_INVALID); Line 76: } Line 79: return getDbFacade().getGlusterGeoRepDao(); Line 80: } Line 81: Line 82: protected GlusterVolumeEntity getSlaveVolume() { Line 83: return getGlusterVolumeDao().getByName(getVdsDAO().getByName(getParameters().getSlaveHost()).getVdsGroupId(), > possible NPE, as you haven't checked if slaveHost is null or not in canDoAc Done Line 84: getParameters().getSlaveVolumeName()); Line 85: } Line 86: Line 87: private boolean areAllRemoteServersUp() { Line 112: Line 113: @Override Line 114: protected void executeCommand() { Line 115: boolean rootSession = getParameters().getUserName().equalsIgnoreCase("root"); Line 116: boolean canProceed = true; > why not call this succeeded? :) Done Line 117: if (!rootSession) { Line 118: canProceed = setUpMountBrokerOnSlaves(); Line 119: } Line 120: if (canProceed) { 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? Involves calls to db and hence in Junit, it will throw NPE's, as at the time of command creation/spy the mocks won't be available. 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 69: doReturn(getVolume(slaveVolumeName, GlusterStatus.UP)).when(command).getSlaveVolume(); Line 70: doReturn(getVolume("", GlusterStatus.UP)).when(command).getGlusterVolume(); Line 71: doReturn(vdsDao).when(command).getVdsDAO(); Line 72: doReturn(volumeDao).when(command).getGlusterVolumeDao(); Line 73: doReturn(geoRepDao).when(command).getGeoRepDao(); > you could move these common expectations to a setup method that you can cal Done Line 74: doReturn(null).when(geoRepDao).getGeoRepSession(any(Guid.class), any(String.class), any(String.class)); Line 75: assertTrue(command.canDoAction()); Line 76: } Line 77: 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 But I want the status, vdsGroupId and also id to be set. 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 22: this.geoRepSessionId = geoRepSessionId; Line 23: } Line 24: Line 25: public GlusterVolumeGeoRepSessionParameters(Guid volumeId, String slaveVolumeName, String slaveHost) { Line 26: this(volumeId, slaveVolumeName, slaveHost, "root", null, false); > pass userName, as you have initialised it Done Line 27: } Line 28: Line 29: public GlusterVolumeGeoRepSessionParameters(Guid volumeId, Line 30: String slaveVolumeName, -- 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 <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: anmolbabu <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
