Sahina Bose has posted comments on this change.

Change subject: gluster: bll command for start/stop of services
......................................................................


Patch Set 3: (6 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/ManageGlusterServiceCommand.java
Line 87:             return 
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_INVALID_ACTION_TYPE);
Line 88:         }
Line 89: 
Line 90:         if (Guid.isNullOrEmpty(clusterId) && 
Guid.isNullOrEmpty(serverId)) {
Line 91:             return 
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTERID_AND_SERVERID_BOTH_NULL);
Also check if there are UP servers in cluster to perform action
Line 92:         }
Line 93: 
Line 94:         return true;
Line 95:     }


Line 108:         List<GlusterServerService> serviceList = null;
Line 109: 
Line 110:         List<Callable<Pair<VDS, VDSReturnValue>>> taskList = new 
ArrayList<Callable<Pair<VDS, VDSReturnValue>>>();
Line 111:         for (final VDS upServer : servers) {
Line 112:             serviceList = 
getGlusterServerServiceDao().getByServerId(upServer.getId());
I think the serviceList should not come from each server, but from 
gluster_services table based on service type. Because the design is generic and 
may have different services for a server, we do not want to start/stop all of 
these services.

The serviceList building can be moved outside of this method as it will be used 
both for cluster/server - use GlusterServiceDao
Line 113:             final List<String> serviceListStr = new 
ArrayList<String>();
Line 114:             for (GlusterServerService srvc : serviceList) {
Line 115:                 serviceListStr.add(srvc.getServiceName());
Line 116:             }


Line 247:  */
Line 248: class ManageActionDetail {
Line 249:     private VdcBllMessages canDoActionMsg;
Line 250:     private GlusterServiceStatus status;
Line 251:     private VdcBllMessages statusMsg;
statusMsg not used, can be removed?
Line 252:     private VDSCommandType vdsCmdType;
Line 253:     private AuditLogType actionPerformedActionLog;
Line 254:     private AuditLogType actionFailedActionLog;
Line 255: 


Line 248: class ManageActionDetail {
Line 249:     private VdcBllMessages canDoActionMsg;
Line 250:     private GlusterServiceStatus status;
Line 251:     private VdcBllMessages statusMsg;
Line 252:     private VDSCommandType vdsCmdType;
The VDS command type is always the same for this BLL command, you could remove 
from here.
Line 253:     private AuditLogType actionPerformedActionLog;
Line 254:     private AuditLogType actionFailedActionLog;
Line 255: 
Line 256:     public ManageActionDetail(VdcBllMessages canDoActionMsg,


....................................................
File 
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/ManageGlusterServiceCommandTest.java
Line 131: 
Line 132:         params.setServiceType(ServiceType.SMB);
Line 133:         cmd = spy(new ManageGlusterServiceCommand(params));
Line 134:         assertFalse(cmd.canDoAction());
Line 135: 
You can move these to separate tests. You can also verify the error message 
returned from canDoAction
Line 136:         params.setClusterId(null);
Line 137:         params.setServerId(null);
Line 138:         assertFalse(cmd.canDoAction());
Line 139:     }


Line 157:         cmd = spy(new ManageGlusterServiceCommand(new 
GlusterServiceParameters(Guid.NewGuid(), Guid.NewGuid(), ServiceType.NFS, 
"start")));
Line 158:         setUpMockUpForStart();
Line 159:         cmd.executeCommand();
Line 160:         assertEquals(cmd.getAuditLogTypeValue(), 
AuditLogType.GLUSTER_SERVICE_START_FAILED);
Line 161: 
Again, split to different tests
Line 162:         cmd = spy(new ManageGlusterServiceCommand(new 
GlusterServiceParameters(Guid.NewGuid(), Guid.NewGuid(), ServiceType.NFS, 
"stop")));
Line 163:         setUpMockUpForStop();
Line 164:         cmd.executeCommand();
Line 165:         assertEquals(cmd.getAuditLogTypeValue(), 
AuditLogType.GLUSTER_SERVICE_STOP_FAILED);


--
To view, visit http://gerrit.ovirt.org/14831
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcab38866c49c6f5d43e3b33006c428ec9304501
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shubhendu Tripathi <shtri...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches

Reply via email to