Yair Zaslavsky has posted comments on this change. Change subject: engine: VDS commands for volume geo-rep status and detail ......................................................................
Patch Set 10: (3 comments) http://gerrit.ovirt.org/#/c/31799/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/gluster/GlusterVolumeGeoRepSessionVDSParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/vdscommands/gluster/GlusterVolumeGeoRepSessionVDSParameters.java: Line 3: import org.ovirt.engine.core.compat.Guid; Line 4: Line 5: public class GlusterVolumeGeoRepSessionVDSParameters extends GlusterVolumeVDSParameters { Line 6: Line 7: String slaveHost; why package visibility and not private or protected? Line 8: String slaveVolume; Line 9: Line 10: public GlusterVolumeGeoRepSessionVDSParameters() { Line 11: http://gerrit.ovirt.org/#/c/31799/10/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeGeoRepStatusForXmlRpc.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/gluster/GlusterVolumeGeoRepStatusForXmlRpc.java: Line 45: } Line 46: String masterBrickDir = (innerMap.containsKey(MASTER_BRICK)) ? innerMap.get(MASTER_BRICK).toString() : null; Line 47: GlusterServer glusterServer = getDbUtils().getServerByUuid(masterNodeGlusterId); Line 48: if (glusterServer != null) { Line 49: GlusterBrickEntity brick = Not sure how much I like the fact that DAO code is used at the StatusForXMlRpc class. Do we have other places in code (well, maybe not in gluster :) ) where we do that as well? I thought that at most StatusForXMlRpc classes at most contain data after simple conversion (like simple type conversion), and not fetch from db. Line 50: getDbUtils().getGlusterBrickByServerUuidAndBrickDir(glusterServer.getId(), masterBrickDir); Line 51: if (brick != null) { Line 52: details.setMasterBrickId(brick.getId()); Line 53: } http://gerrit.ovirt.org/#/c/31799/10/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/VdsmErrors.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/VdsmErrors.java: Line 802: Line 803: @DefaultStringValue("Failed to get gluster volume geo-replication status") Line 804: String GlusterVolumeGeoRepStatusFailed(); Line 805: Line 806: @DefaultStringValue("Failed to get gluster volume geo-replication status detail") i hink that both here and in vdsmerrors.properties - the english text has some room to improvement when it comes to grammar. I realize the idea for this came from the verb name, but as a webadmin user, i would like to see proper grammar. What do you think? Line 807: String GlusterVolumeGeoRepStatusDetailFailed(); Line 808: Line 809: @DefaultStringValue("Failed to get status of gluster volume remove bricks") Line 810: String GlusterVolumeRemoveBrickStatusFailed(); -- To view, visit http://gerrit.ovirt.org/31799 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id313742c9f3c036a0017fe37bd818af1bed0e081 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sahina Bose <[email protected]> Gerrit-Reviewer: Kanagaraj M <[email protected]> Gerrit-Reviewer: Ramesh N <[email protected]> Gerrit-Reviewer: Sahina Bose <[email protected]> Gerrit-Reviewer: Shubhendu Tripathi <[email protected]> Gerrit-Reviewer: Yair Zaslavsky <[email protected]> Gerrit-Reviewer: anmolbabu <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
