Shireesh Anjal has posted comments on this change.
Change subject: engine: Brick existance validation for add brick
......................................................................
Patch Set 1: (7 inline comments)
Few comments, mostly related to naming and duplicate variables.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/AddBricksToGlusterVolumeCommand.java
Line 186: private boolean isValidBricks(List<GlusterBrickEntity> newBricks)
{
minor comment - since we are validating multiple bricks, 'is' doesn't sound
good to me. How about 'areBricksValid' or simply 'validateBricks' ?
Line 198: private boolean brickExist(GlusterBrickEntity newBrick) {
another small naming comment - 'brickExists' sounds better?
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dal/VdcBllMessages.java
Line 622: ACTION_TYPE_FAILED_BRICK_ALREADY_EXIST_IN_VOLUME,
Since we are talking about a single brick, it would be better to name this as
ACTION_TYPE_FAILED_BRICK_ALREADY_EXISTS_IN_VOLUME
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 704: VAR__TYPE__GLUSTER_BRICK=$type Gluster Brick
What is the need to have two different types VAR__TYPE__GLUSTER_VOLUME_BRICK
and VAR__TYPE__GLUSTER_BRICK?
I agree that the description "Gluster Brick" sounds better than the lengthy
"Gluster Volume Brick", but essentially we are referring to the same entity,
and hence we should just change the description of the existing type instead of
introducing a new one.
Line 754: ACTION_TYPE_FAILED_BRICK_ALREADY_EXIST_IN_VOLUME=Cannot ${action}
${type}. Brick ${brick} already used by the volume ${volumeName}.
Similar to an earlier comment, I would prefer the name to be
ACTION_TYPE_FAILED_BRICK_ALREADY_EXISTS_IN_VOLUME
Also, removing _ALREADY_ can make it smaller, without losing the meaning.
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java
Line 698:
Similar to an earlier comment, is this really required when we aleady have
VAR__TYPE__GLUSTER_VOLUME_BRICK ?
Line 1955: String ACTION_TYPE_FAILED_BRICK_ALREADY_EXIST_IN_VOLUME();
Similar to an earlier comment, I would prefer the name to be
ACTION_TYPE_FAILED_BRICK_ALREADY_EXISTS_IN_VOLUME Also, removing _ALREADY_ can
make it smaller, without losing the meaning.
--
To view, visit http://gerrit.ovirt.org/5432
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie93eb97560bc02157683983aa4e65cfb187bdb97
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Selvasundaram <[email protected]>
Gerrit-Reviewer: Omer Frenkel <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
