Shireesh Anjal has posted comments on this change.
Change subject: [WIP] engine: Get Volume Status Query
......................................................................
Patch Set 1: (28 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterVolumeStatusQuery.java
Line 11:
Line 12: /**
Line 13: * Query to get given volume status
Line 14: */
Line 15: public class GetGlusterVolumeStatusQuery<P extends
GlusterVolumeStatusParameters> extends QueriesCommandBase<P> {
Though the gluster cli command is called 'gluster volume status', I think it
generally sounds misleading - as if we it just provides the 'status' of the
volume, whereas what it provides is a lot more details. How about changing name
of this query to "GetGlusterVolumeAdvancedDetailsQuery" ?
Line 16:
Line 17: public GetGlusterVolumeStatusQuery(P params) {
Line 18: super(params);
Line 19: }
Line 31: public ClusterUtils getClusterUtils() {
Line 32: return ClusterUtils.getInstance();
Line 33: }
Line 34:
Line 35: public VDSBrokerFrontend getBackendInstance() {
What you are actually returning from this method is not backend instance, but
resource manager.
Line 36: return Backend.getInstance()
Line 37: .getResourceManager();
Line 38: }
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/BrickDetails.java
Line 3: import java.util.List;
Line 4:
Line 5: /**
Line 6: * GlusterFS provides lot of volume status information about the volume
brick.
Line 7: * This will consolidate all the status inforamtion
How about "This class encapsulates advanced details of a brick, fetched using
the 'gluster volume status' command"
Line 8: *
Line 9: * @see BrickProperties
Line 10: * @see GlusterClientInfo
Line 11: * @see MemoryStatus
Line 10: * @see GlusterClientInfo
Line 11: * @see MemoryStatus
Line 12:
Line 13: */
Line 14: public class BrickDetails extends BrickProperties {
I think BrickProperties can be a member variable inside BrickDetails
Line 15:
Line 16: private static final long serialVersionUID = -1134758927239004412L;
Line 17:
Line 18: public BrickDetails() {
Line 17:
Line 18: public BrickDetails() {
Line 19: }
Line 20:
Line 21: List<GlusterClientInfo> clientInfo;
I would suggest to rename this variable as "clients"
Line 22: MemoryStatus memoryStatus;
Line 23:
Line 24: public List<GlusterClientInfo> getClientInfo() {
Line 25: return clientInfo;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/BrickProperties.java
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8:
Line 9: /**
Line 10: * The gluster volume status detail.
Please correct the comment.
Line 11: *
Line 12: */
Line 13: public class BrickProperties {
Line 14:
Line 11: *
Line 12: */
Line 13: public class BrickProperties {
Line 14:
Line 15: @NotNull(message = "VALIDATION.GLUSTER.BRICK.ID.NOT_NULL", groups
= { RemoveBrick.class })
When will this validation annotation be used?
Line 16: private Guid brickId;
Line 17:
Line 18: private Integer port;
Line 19:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterClientInfo.java
Line 14: private Integer clientPort;
Line 15:
Line 16: private Integer bytesRead;
Line 17:
Line 18: private Integer bytesWrite;
Should this be bytesWritten ?
Line 19:
Line 20: public GlusterClientInfo() {
Line 21: }
Line 22:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterMallInfo.java
Line 4: /**
Line 5: * The gluster volume memory status Mall Info.
Line 6: *
Line 7: */
Line 8: public class GlusterMallInfo{
Nothing specific to Gluster. So I think this class can be called just MallInfo
Line 9:
Line 10: private Integer arena;
Line 11:
Line 12: private Integer ordblks;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterMempool.java
Line 4: /**
Line 5: * The gluster volume memory status Mem pool.
Line 6: *
Line 7: */
Line 8: public class GlusterMempool{
Not specific to Gluster. So I think this class can be called just MemPool
Line 9:
Line 10: private static final long serialVersionUID = 4426819375609665363L;
Line 11:
Line 12: private String name;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeStatusEntity.java
Line 16: * @see GlusterClientInfo
Line 17: * @see MemoryStatus
Line 18:
Line 19: */
Line 20: public class GlusterVolumeStatusEntity implements BusinessEntity<Guid>
{
I think GlusterVolumeAdvancedDetails is a better name for this class. Need not
implement BusinessEntity.
Line 21:
Line 22: private static final long serialVersionUID = -1134758927239004412L;
Line 23:
Line 24: public GlusterVolumeStatusEntity() {
Line 23:
Line 24: public GlusterVolumeStatusEntity() {
Line 25: }
Line 26:
Line 27: @NotNull(message = "VALIDATION.GLUSTER.VOLUME.ID.NOT_NULL", groups
= { RemoveEntity.class })
When will this validation annotation be used?
Line 28: private Guid volumeId;
Line 29:
Line 30: List<BrickDetails> brickDetails;
Line 31: List<VolumeStatusNfs> volumeStatusNfs;
Line 28: private Guid volumeId;
Line 29:
Line 30: List<BrickDetails> brickDetails;
Line 31: List<VolumeStatusNfs> volumeStatusNfs;
Line 32: List<VolumeStatusShd> volumeStatusShd;
Even though these are returned by the gluster cli command 'gluster volume
status', these are essentially cluster level details. So the class and variable
names need not contain "VolumeStatus". In fact these are general information
about two services - NFS and SHD. So I would suggest to have a single generic
class ServiceInfo, and name the two variables as nfsServices and shdServices.
Line 33:
Line 34: public Guid getId() {
Line 35: return volumeId;
Line 36: }
Line 30: List<BrickDetails> brickDetails;
Line 31: List<VolumeStatusNfs> volumeStatusNfs;
Line 32: List<VolumeStatusShd> volumeStatusShd;
Line 33:
Line 34: public Guid getId() {
Should the getter and setter be called getVolumeId() and setVolumeId() ?
Line 35: return volumeId;
Line 36: }
Line 37:
Line 38: public void setId(Guid volumeId) {
Line 62: public void setVolumeStatusShd(List<VolumeStatusShd>
volumeStatusShd) {
Line 63: this.volumeStatusShd = volumeStatusShd;
Line 64: }
Line 65:
Line 66: public class VolumeStatusNfs {
Rename this to ServiceInfo and keep it in a separate source file of itself. The
SHD service information can also be captured in objects of the same class,
where port number can remain null if not provided by the gluster command.
Line 67: public VolumeStatusNfs() {
Line 68:
Line 69: }
Line 70:
Line 141: }
Line 142: }
Line 143:
Line 144: public void copyDetailsFrom(GlusterVolumeStatusEntity entity) {
Line 145: // Copy all the fields which received for "DETAIL"
I think the comment is not adding much value here.
Line 146: for (BrickDetails entityBrick : entity.getBrickDetails()) {
Line 147: for (BrickDetails brick : getBrickDetails()) {
Line 148: if
(entityBrick.getBrickId().equals(brick.getBrickId())) {
Line 149: brick = copyBrickDetails(brick, entityBrick);
Line 162: return brick;
Line 163: }
Line 164:
Line 165: public void copyClientsFrom(GlusterVolumeStatusEntity entity) {
Line 166: // Copy all the fields which received for "Clients"
Same here, the comment doesn't seem to add much value.
Line 167: for (BrickDetails entityBrick : entity.getBrickDetails()) {
Line 168: for (BrickDetails brick : getBrickDetails()) {
Line 169: if
(entityBrick.getBrickId().equals(brick.getBrickId())) {
Line 170: brick = copyClientDetails(brick, entityBrick);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/GlusterVolumeStatusOption.java
Line 1: package org.ovirt.engine.core.common.businessentities.gluster;
Line 2:
Line 3:
Line 4: /**
Line 5: * Enum of Gluster volume Statuses
A better explanation of the enum is required here.
Line 6: *
Line 7: */
Line 8: public enum GlusterVolumeStatusOption {
Line 9: DETAIL,
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/gluster/MemoryStatus.java
Line 8: *
Line 9: */
Line 10: public class MemoryStatus{
Line 11:
Line 12: public MemoryStatus() {
I guess this constructor can be removed.
Line 13: }
Line 14:
Line 15: private GlusterMallInfo glusterMallInfo;
Line 16:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/gluster/GlusterVolumeStatusParameters.java
Line 5: /**
Line 6: * Parameter class with volume name, brick name and details as
parameters. <br>
Line 7: * This will be used by gluster volume status query. <br>
Line 8: */
Line 9: public class GlusterVolumeStatusParameters extends GlusterParameters {
I would recommend to change this class name to
GlusterVolumeAdvancedDetailsParameters
Line 10: private static final long serialVersionUID = -1224829720081853632L;
Line 11: private String volumeName;
Line 12: private String brickName;
Line 13: private boolean details;
Line 9: public class GlusterVolumeStatusParameters extends GlusterParameters {
Line 10: private static final long serialVersionUID = -1224829720081853632L;
Line 11: private String volumeName;
Line 12: private String brickName;
Line 13: private boolean details;
how about detailsRequired ?
Line 14:
Line 15: public GlusterVolumeStatusParameters(Guid clusterId, String
volumeName, String brickName, boolean details) {
Line 16: super(clusterId);
Line 17: setVolumeName(volumeName);
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/gluster/GlusterVolumeStatusVDSParameters.java
Line 8: */
Line 9: public class GlusterVolumeStatusVDSParameters extends
GlusterVolumeVDSParameters {
Line 10: private final Guid clusterId;
Line 11: private final String brickName;
Line 12: private final boolean details;
detailedRequired would be a better name
Line 13:
Line 14: public GlusterVolumeStatusVDSParameters(Guid upServerId,
Line 15: Guid clusterId,
Line 16: String volumeName,
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GetGlusterVolumeStatusVDSCommand.java
Line 44: if (getSucceeded()) {
Line 45:
volumeStatusEntity.copyClientsFrom(result.getVolumeStatus());
Line 46:
Line 47: result =
Line 48:
executeVolumeStatusInfo(volumeStatusEntity, GlusterVolumeStatusOption.MEM.name()
If the sole purpose of this enum is to pass appropriate parameters to the vdsm
command, we could very well have the enum names in lower case and avoid the
call to toLowerCase() here.
Line 49: .toLowerCase());
Line 50: if (getSucceeded()) {
Line 51:
volumeStatusEntity.copyMemoryFrom(result.getVolumeStatus());
Line 52: }
Line 54: }
Line 55: }
Line 56: }
Line 57:
Line 58: ProceedProxyReturnValue();
This method needs to be called immediately after executing the vdsm command, to
avoid further processing in case of unexpected failures.
Line 59: setReturnValue(volumeStatusEntity);
Line 60: }
Line 61:
Line 62: private GlusterVolumeStatusReturnForXmlRpc
executeVolumeStatusInfo(GlusterVolumeStatusEntity entity,
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/irsbroker/GlusterVolumeStatusReturnForXmlRpc.java
Line 72: // We are ignoring missing fields after the status, because on
failure it is
Line 73: // not sent.
Line 74: // [XmlRpcMissingMapping(MappingAction.Ignore),
XmlRpcMember("volumeStatus")]
Line 75:
Line 76: // [XmlRpcMember("status")]
I guess these comments are not required.
Line 77: public StatusForXmlRpc mStatus;
Line 78:
Line 79: GlusterVolumeStatusEntity volumeStatusEntity;
Line 80: List<BrickDetails> brickDetails;
Line 75:
Line 76: // [XmlRpcMember("status")]
Line 77: public StatusForXmlRpc mStatus;
Line 78:
Line 79: GlusterVolumeStatusEntity volumeStatusEntity;
Mark this a private? Since the volume advanced details is the only object to be
exposed publicly, I think other variables need not be defined at class level.
The should rather be populated inside this main object by the corresponding
methods.
Line 80: List<BrickDetails> brickDetails;
Line 81: private GlusterVolumeEntity volume;
Line 82: List<VolumeStatusNfs> volumeStatusNfs;
Line 83: List<VolumeStatusShd> volumeStatusShd;
Line 252:
Line 253: private Guid getBrickId(List<GlusterBrickEntity> bricksList,
String qualifiedBrickName) {
Line 254: String[] brickParts = qualifiedBrickName.split(":", -1);
Line 255:
Line 256: if (brickParts.length != 2) {
This validation is not required. You can expect the brick name returned by
glusterfs (and consequently vdsm) to be of the correct format.
Line 257: throw new RuntimeException("Invalid brick representation
[" + qualifiedBrickName + "]");
Line 258: }
Line 259: for (GlusterBrickEntity brick : bricksList) {
Line 260: // Compare the brickname with the existing volume brick
Line 261: if (brick.getQualifiedName().equals(qualifiedBrickName)) {
Line 262: return brick.getId();
Line 263: }
Line 264: }
Line 265: return null;
It is possible that the brick is not found, in cases where user has added more
bricks to the volume directly using gluster CLI (and this change has not been
synced yet by the GlusterManager). It is ok to return null in such cases,
though it will be good to add a comment to that effect.
Line 266: }
Line 267:
Line 268: protected GlusterVolumeDao getGlusterVolumeDao() {
Line 269: return DbFacade.getInstance().getGlusterVolumeDao();
--
To view, visit http://gerrit.ovirt.org/7807
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If6b590eaebeb1d06b7278300d5e12b2dab9eb093
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Dhandapani Gopal <[email protected]>
Gerrit-Reviewer: Kanagaraj M <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
