Shubhendu Tripathi has posted comments on this change.
Change subject: engine:BLL Command to Start Remove Gluster volume brick
......................................................................
Patch Set 5:
(17 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/StartRemoveGlusterVolumeBricksCommand.java
Line 49: protected boolean canDoAction() {
Line 50: if (!super.canDoAction()) {
Line 51: return false;
Line 52: }
Line 53: if (getParameters().getBricks() == null ||
getParameters().getBricks().size() == 0) {
You may think of using isEmpty() method here
Line 54:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_BRICKS_REQUIRED);
Line 55: return false;
Line 56: }
Line 57: if (getGlusterVolume().getBricks().size() == 1 ||
Line 63: || getGlusterVolume().getVolumeType() ==
GlusterVolumeType.DISTRIBUTED_REPLICATE) {
Line 64: if (getParameters().getReplicaCount() <
getGlusterVolume().getReplicaCount() - 1) {
Line 65:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CAN_NOT_REDUCE_REPLICA_COUNT_MORE_THAN_ONE);
Line 66: return false;
Line 67: } else if (getParameters().getReplicaCount() >
getGlusterVolume().getReplicaCount()) {
Shouldnt we check these conditions exclusively and throw errors? Check this.
Line 68:
addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_CAN_NOT_INCREASE_REPLICA_COUNT);
Line 69: return false;
Line 70: }
Line 71: }
Line 98:
getReturnValue().setActionReturnValue(returnValue.getReturnValue());
Line 99: }
Line 100:
Line 101: protected void updateBricksWithTaskID(GlusterAsyncTask asyncTask)
{
Line 102: for (GlusterBrickEntity brickEntity :
getParameters().getBricks()) {
We might need to fire these updates as part of single transaction
Line 103: getGlusterBrickDao().updateBrickTask(brickEntity.getId(),
asyncTask.getTaskId());
Line 104: }
Line 105: getGlusterVolumeDao().updateVolumeTask(getGlusterVolumeId(),
asyncTask.getTaskId());
Line 106: }
Line 112: * @param bricks
Line 113: * The bricks to validate
Line 114: * @return true if all bricks have valid ids, else false
Line 115: */
Line 116: private boolean validateBricks(List<GlusterBrickEntity> bricks) {
This method is not only validating the bricks list but also adds entries to
bricks list. See if the these two logics can be broken in separate methods
Line 117: GlusterVolumeEntity volume = getGlusterVolume();
Line 118: for (GlusterBrickEntity brick : bricks) {
Line 119: if (brick.getServerName() != null &&
brick.getBrickDirectory() != null) {
Line 120: // brick already contains required info.
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 261:
RemoveGlusterHook(1418,ActionGroup.MANIPULATE_GLUSTER_HOOK,QuotaDependency.NONE),
Line 262: RefreshGlusterHooks(1419, ActionGroup.MANIPULATE_GLUSTER_HOOK,
QuotaDependency.NONE),
Line 263: ManageGlusterService(1420,
ActionGroup.MANIPULATE_GLUSTER_SERVICE, QuotaDependency.NONE),
Line 264: StopRebalanceGlusterVolume(1421,
ActionGroup.MANIPULATE_GLUSTER_VOLUME,false, QuotaDependency.NONE),
Line 265: StartRemoveGlusterVolumeBrick(1422,
ActionGroup.MANIPULATE_GLUSTER_VOLUME, QuotaDependency.NONE),
Shouldnt it be called StartRemoveGlusterVolumeBricks?
Line 266: // Cluster Policy
Line 267: AddClusterPolicy(1450,
ActionGroup.EDIT_STORAGE_POOL_CONFIGURATION, false, QuotaDependency.NONE),
Line 268: EditClusterPolicy(1451,
ActionGroup.EDIT_STORAGE_POOL_CONFIGURATION, false, QuotaDependency.NONE),
Line 269: RemoveClusterPolicy(1452,
ActionGroup.EDIT_STORAGE_POOL_CONFIGURATION, false, QuotaDependency.NONE),
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/asynctasks/gluster/GlusterTaskType.java
Line 6: import org.ovirt.engine.core.common.job.StepEnum;
Line 7:
Line 8: public enum GlusterTaskType {
Line 9: REBALANCE_VOLUME(StepEnum.REBALANCING_VOLUME),
Line 10:
REMOVING_GLUSTER_VOLUME_BIRCK(StepEnum.REMOVING_GLUSTER_VOLUME_BIRCK),
I think REMOVING_BRICKS is enough here.
Line 11: ;
Line 12:
Line 13: private StepEnum step;
Line 14: private static Map<StepEnum, GlusterTaskType> mappings;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java
Line 82: VAR__ACTION__LOGON,
Line 83: VAR__ACTION__LOGOFF,
Line 84: VAR__ACTION__REBALANCE_START,
Line 85: VAR__ACTION__REBALANCE_STOP,
Line 86: VAR__ACTION__START_REMOVE,
Shouldnt it be called VAR__ACTION__REMOVE_BRICK_START?
Also there should be similar entries for VAR__ACTION__REMOVE_BRICK_STOP,
VAR__ACTION__REMOVE_BRICK_COMMIT and VAR__ACTION__REMOVE_BRICK_STATUS
Line 87: VAR__ACTION__ASSIGN,
Line 88: VAR__ACTION__START_PROFILE,
Line 89: VAR__ACTION__STOP_PROFILE,
Line 90: VAR__ACTION__ENABLE,
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/job/StepEnum.java
Line 22:
Line 23: // Gluster
Line 24: SETTING_GLUSTER_OPTION,
Line 25: REBALANCING_VOLUME,
Line 26: REMOVING_GLUSTER_VOLUME_BIRCK,
REMOVING_BRICKS should be enough
Line 27:
Line 28: /**
Line 29: * Maps VDSM tasks type to {@code StepEnum} so it can be
resolvable as readable description
Line 30: */
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 293: VAR__ACTION__LOGOFF=$action log off
Line 294: VAR__ACTION__ASSIGN=$action assign
Line 295: VAR__ACTION__REBALANCE_START=$action rebalance
Line 296: VAR__ACTION__REBALANCE_STOP=$action stop rebalance
Line 297: VAR__ACTION__START_REMOVE=$action start removing
to be in sync with REBALANCE, it should be called
VAR__ACTION__REMOVE_BRICKS_START. Similarly entries for
VAR__ACTION__REMOVE_BRICKS_STOP, VAR__ACTION__REMOVE_BRICKS_COMMIT and
VAR__ACTION__REMOVE_BRICKS_STATUS would be added
Line 298: VAR__ACTION__START_PROFILE=$action start profiling
Line 299: VAR__ACTION__STOP_PROFILE=$action stop profiling
Line 300: VAR__ACTION__ENABLE=$action enable
Line 301: VAR__ACTION__DISABLE=$action disable
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 610: GLUSTER_VOLUME_OPTIONS_RESET_ALL=All Volume Options reset on
${glusterVolumeName}.
Line 611: GLUSTER_VOLUME_OPTIONS_RESET_FAILED=Could not reset Gluster Volume
${glusterVolumeName} Options.
Line 612: GLUSTER_VOLUME_DELETE=Gluster Volume ${glusterVolumeName} deleted.
Line 613: GLUSTER_VOLUME_DELETE_FAILED=Could not delete Gluster Volume
${glusterVolumeName}.
Line 614: GLUSTER_VOLUME_REMOVE_BRICKS=Bricks removed from Gluster Volume
${glusterVolumeName}.
I think it would be better to show no of deleted bricks as well, as part of
this message
Line 615: GLUSTER_VOLUME_REMOVE_BRICKS_FAILED=Could not remove Gluster Volume
${glusterVolumeName} Bricks.
Line 616: GLUSTER_VOLUME_ADD_BRICK= ${NoOfBricks} brick(s) added to volume
${glusterVolumeName}.
Line 617: GLUSTER_VOLUME_ADD_BRICK_FAILED=Gluster Volume ${glusterVolumeName}
add brick failed.
Line 618: GLUSTER_VOLUME_REBALANCE_START=Gluster Volume ${glusterVolumeName}
rebalance started.
Line 619: GLUSTER_VOLUME_REBALANCE_START_FAILED=Could not start Gluster Volume
${glusterVolumeName} rebalance.
Line 620: GLUSTER_VOLUME_REBALANCE_STOP=Gluster Volume ${glusterVolumeName}
rebalance stopped.
Line 621: GLUSTER_VOLUME_REBALANCE_STOP_FAILED=Could not stop rebalance of
gluster volume ${glusterVolumeName}.
Line 622: START_REMOVING_GLUSTER_VOLUME_BRICKS=Started removing brick from
Gluser Volume ${glusterVolumeName}
Line 623: START_REMOVING_GLUSTER_VOLUME_BRICKS_FAILED=Could not remove brick
from Gluser Volume ${glusterVolumeName}
The message should be "Could not start remove brick from Gluster Volume <Volume
Name>"
Line 624: GLUSTER_VOLUME_REPLACE_BRICK_FAILED=Replace Gluster Volume Brick
failed
Line 625: GLUSTER_VOLUME_REPLACE_BRICK_START=Gluster Volume
${glusterVolumeName} Replace Brick started.
Line 626: GLUSTER_VOLUME_REPLACE_BRICK_START_FAILED=Could not start Gluster
Volume ${glusterVolumeName} Replace Brick.
Line 627: GLUSTER_SERVER_ADD_FAILED=Failed to add gluster server ${VdsName}
into Cluster ${VdsGroupName}.
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/ExecutionMessages.properties
Line 131:
Line 132: # Gluster step types
Line 133: step.SETTING_GLUSTER_OPTION=Setting option ${Key}=${Value} on volume
${GlusterVolume} of cluster ${Cluster}
Line 134: step.REBALANCING_VOLUME=Rebalancing Gluster Volume ${GlusterVolume}
in Cluster ${Cluster}.( ${Status} ${info})
Line 135: step.REMOVING_GLUSTER_VOLUME_BIRCK = Removing Bricks from Gluster
Volume ${GlusterVolume} in Cluster ${Cluster}.( ${Status} ${info})
step.REMOVING_BRICKS ?? Just a suggestion.
Line 136:
Line 137: # Non-monitored job:
Line 138: job.AddVmInterface=Adding Network Interface ${InterfaceName} to VM
${VM}
Line 139: job.RemoveVmInterface=Removing Network Interface ${InterfaceName}
from VM ${VM}
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java
Line 805: @DefaultStringValue("$action stop rebalance")
Line 806: String VAR__ACTION__REBALANCE_STOP();
Line 807:
Line 808: @DefaultStringValue("$action start removing")
Line 809: String VAR__ACTION__START_REMOVE();
VAR__ACTION__REMOVE_BRICKS_START. Sync with AppErrors.properties
Line 810:
Line 811: @DefaultStringValue("$action start profiling")
Line 812: String VAR__ACTION__START_PROFILE();
Line 813:
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/LocalizedEnums.java
Line 257: String AuditLogType___GLUSTER_VOLUME_REMOVE_BRICKS();
Line 258:
Line 259: String AuditLogType___GLUSTER_VOLUME_REMOVE_BRICKS_FAILED();
Line 260:
Line 261: String AuditLogType___START_REMOVING_GLUSTER_VOLUME_BRICKS();
What about renaming to AuditLogType___GLUSTER_VOLUME_REMOVE_BRICKS_START?? It
would be in sync with other gluster entries.
Line 262:
Line 263: String
AuditLogType___START_REMOVING_GLUSTER_VOLUME_BRICKS_FAILED();
Line 264:
Line 265: String AuditLogType___GLUSTER_VOLUME_ADD_BRICK();
Line 259: String AuditLogType___GLUSTER_VOLUME_REMOVE_BRICKS_FAILED();
Line 260:
Line 261: String AuditLogType___START_REMOVING_GLUSTER_VOLUME_BRICKS();
Line 262:
Line 263: String
AuditLogType___START_REMOVING_GLUSTER_VOLUME_BRICKS_FAILED();
What about renaming to
AuditLogType___GLUSTER_VOLUME_REMOVE_BRICKS_START_FAILED?? It would be in sync
with other gluster entries.
Line 264:
Line 265: String AuditLogType___GLUSTER_VOLUME_ADD_BRICK();
Line 266:
Line 267: String AuditLogType___GLUSTER_VOLUME_ADD_BRICK_FAILED();
....................................................
File
frontend/webadmin/modules/uicompat/src/main/resources/org/ovirt/engine/ui/uicompat/LocalizedEnums.properties
Line 125: AuditLogType___GLUSTER_VOLUME_DELETE_FAILED=Gluster Volume could not
be deleted
Line 126: AuditLogType___GLUSTER_VOLUME_REMOVE_BRICKS=Gluster Volume Bricks
Removed
Line 127: AuditLogType___GLUSTER_VOLUME_REMOVE_BRICKS_FAILED=Gluster Volume
Bricks could not be removed
Line 128: AuditLogType___START_REMOVING_GLUSTER_VOLUME_BRICKS=Started removing
Gluster Volume Bricks
Line 129: AuditLogType___START_REMOVING_GLUSTER_VOLUME_BRICKS_FAILED=Could not
remove Gluster volume bricks
Same as LocalizedEnums.java
Line 130: AuditLogType___GLUSTER_VOLUME_ADD_BRICK=Gluster Volume brick(s) added
Line 131: AuditLogType___GLUSTER_VOLUME_ADD_BRICK_FAILED=Failed to add brick(s)
on Gluster Volume
Line 132: AuditLogType___GLUSTER_VOLUME_REBALANCE_START=Gluster Volume
Rebalance started
Line 133: AuditLogType___GLUSTER_VOLUME_REBALANCE_START_FAILED=Gluster Volume
Rebalance could not be started
....................................................
File
frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 296: VAR__ACTION__LOGON=$action log on
Line 297: VAR__ACTION__LOGOFF=$action log off
Line 298: VAR__ACTION__REBALANCE_START=$action rebalance
Line 299: VAR__ACTION__REBALANCE_STOP=$action stop rebalance
Line 300: VAR__ACTION__START_REMOVE=$action start removing
Same as backend version of AppError.properties
Line 301: VAR__ACTION__START_PROFILE=$action start profiling
Line 302: VAR__ACTION__STOP_PROFILE=$action stop profiling
Line 303: VAR__ACTION__ASSIGN=$action assign
Line 304: VAR__ACTION__REFRESH=$action refresh
--
To view, visit http://gerrit.ovirt.org/18923
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie3ee4620b75b4b714087dbf1dec3720661a5ce6b
Gerrit-PatchSet: 5
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Ramesh N <rnach...@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: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
