Omer Frenkel has posted comments on this change.

Change subject: gluster: Bll command to refresh gluster hooks in engine
......................................................................


Patch Set 1: (3 inline comments)

....................................................
File 
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/RefreshGlusterHooksCommand.java
Line 27:     }
Line 28: 
Line 29:     @Override
Line 30:     protected boolean canDoAction() {
Line 31:         if (getParameters().getClusterId() == null) {
using command line user can provide a wrong id (say, by mistake), so its better 
to check for getVdsGroup() == null here as well
Line 32:             
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CLUSTER_IS_NOT_VALID);
Line 33:             return false;
Line 34:         }
Line 35: 


Line 45:     }
Line 46: 
Line 47:     @Override
Line 48:     protected void executeCommand() {
Line 49:         VDSGroup cluster = 
getVdsGroupDAO().get(getParameters().getClusterId());
no need to manually get from db, you can just use getVdsGroup()
Line 50:         try {
Line 51:             getSyncJobInstance().refreshHooksInCluster(cluster, true);
Line 52:             setSucceeded(true);
Line 53:         } catch (VdcBLLException e) {


Line 49:         VDSGroup cluster = 
getVdsGroupDAO().get(getParameters().getClusterId());
Line 50:         try {
Line 51:             getSyncJobInstance().refreshHooksInCluster(cluster, true);
Line 52:             setSucceeded(true);
Line 53:         } catch (VdcBLLException e) {
why do you catch the exception? 
 command base catch all exceptions and initialize an error object so the user 
will get translated message
 also, then you dont need to setSucceeded(false) as this is the default
Line 54:             setSucceeded(false);
Line 55:         }
Line 56:     }
Line 57: 


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4713ca800432aee302f913d0c8aa06a35a5b2db2
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Shubhendu Tripathi <shtri...@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