Shireesh Anjal has posted comments on this change.
Change subject: restapi: Rest API for brick advanced details
......................................................................
Patch Set 1: (7 inline comments)
Overall looks good to me. Few minor comments and responses to your questions
in-line.
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/gluster/GlusterBrickDetailsResource.java
Line 7: import org.ovirt.engine.api.model.GlusterBrickAdvancedDetails;
Line 8: import org.ovirt.engine.api.resource.MediaType;
Line 9:
Line 10: @Produces({MediaType.APPLICATION_XML, MediaType.APPLICATION_JSON,
MediaType.APPLICATION_X_YAML})
Line 11: public interface GlusterBrickDetailsResource {
I guess this interface is not required?
Line 12:
Line 13: @GET
Line 14: @Formatted
Line 15: public GlusterBrickAdvancedDetails get();
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/gluster/BackendGlusterBrickResource.java
Line 87: return populateAdvancedDetails(model,entity);
Line 88: }
Line 89:
Line 90: private GlusterBrick populateAdvancedDetails(GlusterBrick model,
GlusterBrickEntity entity) {
Line 91: //TODO: check if advanced details could not be found, should
this be ignored?
This will happen only if the gluster command fails or something goes wrong in
the query code. In either case, an exception will be thrown, and it will be
handled by the base class, probably resulting in HTTP response code 500, which
is good enough.
Since the query doesn't fetch from DB but uses a VDS command to fetch the
details from glusterfs, there is no valid "no records found" kind of a scenario
here.
So I don't think any special handling is required here.
Line 92:
Line 93: GlusterVolumeEntity volumeEntity =
getEntity(GlusterVolumeEntity.class,
Line 94:
VdcQueryType.GetGlusterVolumeById,
Line 95: new
IdQueryParameters(entity.getVolumeId()),
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GlusterBrickDetailMapper.java
Line 19:
Line 20: @Mapping (from=GlusterBrickAdvancedDetails.class,
to=GlusterVolumeAdvancedDetails.class)
Line 21: public static GlusterVolumeAdvancedDetails
map(GlusterBrickAdvancedDetails model, GlusterVolumeAdvancedDetails toEntity) {
Line 22: //AdvancedDetails is a read only from server and no support
for setting these.
Line 23: //Hence mapping from REST model to Business entity not
required.
I guess you can do away with this method itself..
Line 24: GlusterVolumeAdvancedDetails entity = (toEntity == null) ? new
GlusterVolumeAdvancedDetails() : toEntity;
Line 25: return entity;
Line 26: }
Line 27:
Line 30: GlusterBrickAdvancedDetails model = (toModel == null) ? new
GlusterBrickAdvancedDetails() : toModel;
Line 31:
Line 32: if (fromEntity.getBrickDetails() == null) return model;
Line 33: //Since the getDetails call is for a single brick the list
size should always be 1.
Line 34: //TODO: check-Should Exception be thrown if greater?
There is no chance that it will ever contain more than 1 elements. So I'm ok
without this additional check.
Line 35: BrickDetails detail = (fromEntity.getBrickDetails().size() >
0) ? fromEntity.getBrickDetails().get(0) : null;
Line 36:
Line 37: if (detail == null) return model;
Line 38:
Line 36:
Line 37: if (detail == null) return model;
Line 38:
Line 39: if (detail.getBrickProperties() != null) {
Line 40: BrickProperties props = detail.getBrickProperties();
minor suggestion: How about a copyBrickProperties() ?
Line 41: model.setBlockSize(props.getBlockSize());
Line 42: if (StringUtils.isNotEmpty(props.getDevice()))
Line 43: model.setDevice(props.getDevice());
Line 44: model.setFreeSize(props.getFreeSize());
Line 102: MemoryPool poolModel = new MemoryPool();
Line 103:
Line 104: if (poolEntity == null) return null;
Line 105:
Line 106: poolModel.setName(poolEntity.getName());
Is the StringUtils.isNotEmpty() check required here also?
Line 107: poolModel.setAllocCount(poolEntity.getAllocCount());
Line 108: poolModel.setColdCount(poolEntity.getColdCount());
Line 109: poolModel.setHotCount(poolEntity.getHotCount());
Line 110: poolModel.setMaxAlloc(poolEntity.getMaxAlloc());
....................................................
File
backend/manager/modules/restapi/types/src/test/java/org/ovirt/engine/api/restapi/types/GlusterBrickDetailMapperTest.java
Line 83: BrickDetails details = new BrickDetails();
Line 84: BrickProperties props = new BrickProperties();
Line 85: props.setBlockSize(blockSize);
Line 86: props.setPid(88888);
Line 87: props.setMntOptions("mntOption1");
minor: Maybe you can create constants for the pid and mntOptions as well,
similar to blockSize. Or remove blockSize and directly use the hard coded value
here, if all these are used only once.
Line 88: details.setBrickProperties(props);
Line 89: details.setClients(getClientList(clientListSize));
Line 90: details.setMemoryStatus(getMemoryStatus(memPoolSize));
Line 91: list.add(details);
--
To view, visit http://gerrit.ovirt.org/11391
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie219c7cf59fec8a21a54f34959ee5966eed7d524
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sahina Bose <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Shireesh Anjal <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
