Shireesh Anjal has posted comments on this change.
Change subject: engine: Enable gluster hook on cluster
......................................................................
Patch Set 19: (11 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/EnableGlusterHookCommand.java
Line 15: }
Line 16:
Line 17: @Override
Line 18: protected void setActionMessageParameters() {
Line 19:
addCanDoActionMessage(VdcBllMessages.VAR__ACTION__ENABLE_GLUSTER_HOOK);
Similar to start, stop, etc. I think the name of the action enum can be more
generic (VAR__ACTION__ENABLE)
Line 20: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__GLUSTER_HOOK);
Line 21: }
Line 22:
Line 23: @Override
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GlusterHookStatusChangeCommand.java
Line 27: @NonTransactiveCommandAttribute
Line 28: @LockIdNameAttribute(isWait = true)
Line 29: public abstract class GlusterHookStatusChangeCommand extends
GlusterHookCommandBase<GlusterHookParameters> {
Line 30: private static final long serialVersionUID = 1738006110968897175L;
Line 31: private GlusterHookEntity entity;
May I suggest calling this variable as "hook" ?
Line 32:
Line 33: public GlusterHookStatusChangeCommand(GlusterHookParameters
params) {
Line 34: super(params);
Line 35: setVdsGroupId(params.getClusterId());
Line 35: setVdsGroupId(params.getClusterId());
Line 36: }
Line 37:
Line 38: @Override
Line 39: protected BackendInternal getBackend() {
Is this method required?
Line 40: return super.getBackend();
Line 41: }
Line 42:
Line 43: @Override
Line 52:
Line 53: if (Guid.isNullOrEmpty(getParameters().getHookId())) {
Line 54:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_GLUSTER_HOOK_ID_IS_REQUIRED);
Line 55: return false;
Line 56: }
Maybe we should try to fetch the hook from the id, and add return failure if
not found?
Line 57:
Line 58: List <VDS> servers =
getAllUpServers(getParameters().getClusterId());
Line 59: if (servers == null || servers.size() == 0) {
Line 60:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_NO_UP_SERVER_FOUND);
Line 55: return false;
Line 56: }
Line 57:
Line 58: List <VDS> servers =
getAllUpServers(getParameters().getClusterId());
Line 59: if (servers == null || servers.size() == 0) {
Minor: My preference is servers.isEmpty(), thought I don't have a problem with
size() == 0 either.
Line 60:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_NO_UP_SERVER_FOUND);
Line 61: return false;
Line 62: }
Line 63:
Line 100: atLeastOneSuccess = true;
Line 101: // update status in database
Line 102: // if a new server has been detected, the hook entry
needs to be added
Line 103: if (getServerHookFromList(serverHooks,
pairResult.getFirst().getId()) == null) {
Line 104: addServerHook(pairResult.getFirst().getId());
Will we miss adding the server if the status change command failed on it? I
guess we should add it if it fails with "already enabled" error. Not sure what
we should do if there is some other error though. Maybe we should let the sync
job handle such cases.
Line 105: } else {
Line 106: updateServerHookStatusInDb(entity.getId(),
pairResult.getFirst().getId(), getNewStatus());
Line 107: }
Line 108: } else {
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/gluster/EnableGlusterHookCommandTest.java
Line 51: */
Line 52: private EnableGlusterHookCommand cmd;
Line 53:
Line 54: @Rule
Line 55: public MockConfigRule mcr = new MockConfigRule();
fyi, you can also use the variable argument constructor to mock the config
values instead of having a separate method for it - example in
GlusterManagerTest.
Line 56:
Line 57: @Mock
Line 58: private GlusterHooksDao hooksDao;
Line 59:
Line 177: @Test
Line 178: public void canDoActionFailsOnNullCluster() {
Line 179: cmd = spy(new EnableGlusterHookCommand(new
GlusterHookParameters(null, HOOK_ID)));
Line 180: setupMocks();
Line 181: assertFalse(cmd.canDoAction());
It is possible to verify the exact error message as well. example:
UpdateVdsGroupCommandTest#canDoActionFailedWithReason()
Line 182: }
Line 183:
Line 184: @Test
Line 185: public void canDoActionFailsOnNullHookId() {
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 590: GLUSTER_VOLUME_BRICK_REMOVED_FROM_CLI=Detected brick ${brick} removed
from Volume ${glusterVolumeName} of cluster ${VdsGroupName}, and removed it
from engine DB.
Line 591: GLUSTER_SERVER_REMOVED_FROM_CLI=Detected server ${VdsName} removed
from Cluster ${VdsGroupName}, and removed it from engine DB.
Line 592: GLUSTER_VOLUME_STARTED_FROM_CLI=Detected that Volume
${glusterVolumeName} of Cluster ${VdsGroupName} was started, and updated engine
DB with it's new status.
Line 593: GLUSTER_VOLUME_STOPPED_FROM_CLI=Detected that Volume
${glusterVolumeName} of Cluster ${VdsGroupName} was stopped, and updated engine
DB with it's new status.
Line 594: GLUSTER_HOOK_ENABLE=Gluster Hook ${GlusterHookName} enabled.
I don't think ${GlusterHookName} will get printed in the audit log message
unless AuditLogableBase has a getter for it.
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties
Line 88: job.RemoveGlusterServer=Removing Gluster Server ${VDS}
Line 89: job.RegisterDisk=Registering Disk ${DiskAlias}
Line 90: job.AddEventSubscription=Adding subscriber ${Address} to event type
${EventType}
Line 91: job.RemoveEventSubscription=Removing subscriber ${Address} from event
type ${EventType}
Line 92: job.EnableGlusterHook=Enabling Gluster Hook ${GlusterHookName}
I think you need to override getJobMessageProperties() inside the bll command
class for ${GlusterHookName} to get printed.
Line 93:
Line 94: # Step types
Line 95: step.VALIDATING=Validating
Line 96: step.EXECUTING=Executing
....................................................
File
frontend/webadmin/modules/uicompat/src/main/resources/org/ovirt/engine/ui/uicompat/LocalizedEnums.properties
Line 76: EventNotificationEntity___Storage=Storage Management Events:
Line 77: EventNotificationEntity___Engine=General Management Events:
Line 78: EventNotificationEntity___GlusterVolume=Gluster Volume Events:
Line 79: EventNotificationEntity___DWH=Data Warehouse Events:
Line 80: EventNotificationEntity___GlusterHook=Gluster Hook:
Should this be "Gluster Hook Events:" ?
Line 81: AuditLogType___VDS_FAILURE=Host is non-responsive
Line 82: AuditLogType___USER_VDS_MAINTENANCE=Host was switched to Maintenance
Mode
Line 83: AuditLogType___USER_VDS_MAINTENANCE_MIGRATION_FAILED=Failed to switch
Host to Maintenance mode
Line 84: AuditLogType___VDS_MAINTENANCE=Host was switched to Maintenance Mode
--
To view, visit http://gerrit.ovirt.org/10906
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icc6f9c77393ebed2803ec2a1b295a09f61642c31
Gerrit-PatchSet: 19
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Selvasundaram <sesub...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Selvasundaram <sesub...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Yair Zaslavsky <yzasl...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
