Omer Frenkel has posted comments on this change.
Change subject: engine: Gluster Host Remove Command
......................................................................
Patch Set 3: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVdsCommand.java
Line 61: return false;
this method is written in a 'single return point' way,
i'd rather use it (means set returanValue to false instead of return here)
or refactor the method to have multiple return points.
please, if you choose to refactor, have it in a separate patch from the change
this patch is doing.
Line 106: errorType = AuditLogType.USER_FAILED_REMOVE_VDS;
this is not good enough, if for some reason the execution will fail before this
method is called, or will fail not because of this run, there will be no error
for audit log failure.
Line 149: VDSGroup vdsGroup = getVdsGroupDAO().get(getVdsGroupId());
please use getVdsGroup()
Line 175: if (isGlusterEnabled() && !hasVolumeOnServer()) {
why not merge both 'if' checks above to one?
also i would first check ifGlusterEnabled() to save getting all hosts from db
for non gluster hosts.
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterVolumeDaoDbFacadeImpl.java
Line 404: public Integer getBrickCountByServerId(Guid serverId) {
please use stored procedure and add a test to GlusterVolumeDaoTest
--
To view, visit http://gerrit.ovirt.org/4512
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c82837ba911d96a1ab10d11f62d43e9e8ad4d90
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Selvasundaram <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
