Sahina Bose has posted comments on this change. Change subject: engine : add brick to geo-rep slave volume ......................................................................
Patch Set 10: (7 comments) https://gerrit.ovirt.org/#/c/39315/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/AddBricksToGlusterVolumeCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/AddBricksToGlusterVolumeCommand.java: Line 108: return null; Line 109: } Line 110: }); Line 111: } Line 112: freeLock(); what lock are we freeing here? Line 113: if(getGlusterVolume().getIsGeoRepMaster() || getGlusterVolume().getIsGeoRepSlave()) { Line 114: postAddBrickHandleGeoRepCase(bricksList, volumeBeforeBrickAdd); Line 115: } Line 116: } Line 114: postAddBrickHandleGeoRepCase(bricksList, volumeBeforeBrickAdd); Line 115: } Line 116: } Line 117: Line 118: private void postAddBrickHandleGeoRepCase(final List<GlusterBrickEntity> bricksList, final GlusterVolumeEntity volumeBeforeBrickAdd) { Please format lines Line 119: final GlusterVolumeEntity volume = getGlusterVolume(); Line 120: List<GlusterGeoRepSession> sessions = new ArrayList<>(); Line 121: Line 122: //Get all sessions for which the volume is a master Line 124: if(geoRepSessionsForVolumeAsMaster != null && !geoRepSessionsForVolumeAsMaster.isEmpty()) { Line 125: sessions.addAll(geoRepSessionsForVolumeAsMaster); Line 126: } Line 127: //Get all sessions for which the volume is a slave Line 128: List<GlusterGeoRepSession> geoRepSessionsForVolumeAsSlave = getDbFacade().getGlusterGeoRepDao().getGeoRepSessionsForSlaveVolume(volume.getId()); there will only be one session for which volume is slave. You can change the signature of the dao method to reflect that. Line 129: if(geoRepSessionsForVolumeAsSlave != null && !geoRepSessionsForVolumeAsSlave.isEmpty()) { Line 130: sessions.addAll(geoRepSessionsForVolumeAsSlave); Line 131: } Line 132: Line 148: } else { Line 149: //if its a slave volume of current session, newServerIds is just set of new server ids Line 150: newServerIds = findNewServers(bricksList, volumeBeforeBrickAdd); Line 151: if(newServerIds.isEmpty()) { Line 152: return true; return or continue? Line 153: } Line 154: //if its slave and non-root session, do partial mount broker setup as we Line 155: if (!currentSession.getUserName().equalsIgnoreCase("root")) { Line 156: succeeded = evaluateReturnValues(AuditLogType.GLUSTER_GEOREP_SETUP_MOUNT_BROKER_FAILED, command.setUpPartialMountBrokerOnSlaves(newServerIds)); Line 159: succeeded) { : succeeded It's better to convert these into internalCommands and call these rather than calling methods of command class directly, IMO Line 338: return errorType == null ? AuditLogType.GLUSTER_VOLUME_ADD_BRICK_FAILED : errorType; Line 339: } Line 340: } Line 341: Line 342: protected boolean evaluateReturnValues(AuditLogType auditLogType, List<VdcReturnValueBase> returnValues) { seems to be repeated code Line 343: boolean succeeded = true; Line 344: List<String> errors = new ArrayList<>(); Line 345: for (VdcReturnValueBase currentReturnValue : returnValues) { Line 346: boolean currentExecutionStatus = currentReturnValue.getSucceeded(); https://gerrit.ovirt.org/#/c/39315/10/backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterGeoRepDao.java File backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/gluster/GlusterGeoRepDao.java: Line 39: public There will only be 1 session where volume is slave. -- To view, visit https://gerrit.ovirt.org/39315 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id85cb7c3eb35ee84b4eb298cd20d9c657ed608d3 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
