Allon Mureinik has posted comments on this change.
Change subject: core: change vdsSpmId related command to support numerous vdss
......................................................................
Patch Set 6: I would prefer that you didn't submit this
(6 inline comments)
-1 based on a partial review - see inline comments/questions.
I do not have the soruce code in front of me right now, and will probably want
to revist the patch in and review in front of it.
I might add additional comments after that, so your choice if you perefer
relating to the ones given already or wait for a more extensive review.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVdsSpmIdCommand.java
Line 60: //TODO : this logic should be optimized.
Line 61: for (VdsStatic vds : getParameters().getVdss()) {
Line 62: // according to shaharf the first id is 1
Line 63: int selectedId = 1;
Line 64: List<Integer> map = LinqUtils.foreach(vds_spm_id_mapList,
new Function<vds_spm_id_map, Integer>() {
Not a part of this patch, but "map" is a great name for a list variable :-)
Line 65: @Override
Line 66: public Integer eval(vds_spm_id_map vds_spm_id_map) {
Line 67: return vds_spm_id_map.getvds_spm_id();
Line 68: }
Line 74: } else {
Line 75: break;
Line 76: }
Line 77: }
Line 78: vds_spm_id_map newMap = new
vds_spm_id_map(getParameters().getStoragePoolId(), vds.getId(), selectedId);
Maybe I'm missing something here, but it seems as though you're assigning the
SAME vds_spm_id to all of them. AFAIK, this is not the intended behavior.
If I've got it right, the fact that you're doing a batch operation will allow
you can create a set with all the ints from min..max, and use retainsAll() to
find the gaps.
Line 79: DbFacade.getInstance().getVdsSpmIdMapDao().save(newMap);
Line 80: if (getParameters().isCompensationEnabled()) {
Line 81: getCompensationContext().snapshotNewEntity(newMap);
Line 82: getCompensationContext().stateChanged();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ChangeVDSClusterCommand.java
Line 113: return;
Line 114: }
Line 115:
Line 116: VdsSpmIdParameters vdsSpmIdParameters = new
VdsSpmIdParameters();
Line 117:
vdsSpmIdParameters.setVdss(Collections.singletonList(getVds().getStaticData()));
Don't you need the parent command set here too?
Line 118: if (targetStoragePool != null) {
Line 119:
vdsSpmIdParameters.setStoragePoolId(targetStoragePool.getId());
Line 120: VdcReturnValueBase addVdsSpmIdReturn =
Line 121:
Backend.getInstance().runInternalAction(VdcActionType.AddVdsSpmId,
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/VdcActionType.java
Line 70: ApproveVds(112, ActionGroup.CREATE_HOST),
Line 71: HandleVdsCpuFlagsOrClusterChanged(114),
Line 72: InitVdsOnUp(115),
Line 73: SetNonOperationalVds(117),
Line 74: AddMultipleVdsSpmId(119),
what is this related too? Is it possible you forgot to git add the new command?
Line 75: AddVdsSpmId(119),
Line 76: RemoveVdsSpmId(120),
Line 77: // Fencing (including RestartVds above)
Line 78: StartVds(121, ActionGroup.MANIPUTLATE_HOST),
....................................................
Commit Message
Line 8:
Line 9: related to bug https://bugzilla.redhat.com/show_bug.cgi?id=771699
Line 10:
Line 11: currently the different vdsSpmId commands support execution on one vds
Line 12: at a time, which degrades performance. This patch change those commands
s/change/changes/
Line 13: to support execution on numerous hosts at a time.
Line 14:
Line 15: Change-Id: I7531b9edb763d32b96c58104385349d587de3416
Line 11: currently the different vdsSpmId commands support execution on one vds
Line 12: at a time, which degrades performance. This patch change those commands
Line 13: to support execution on numerous hosts at a time.
Line 14:
Line 15: Change-Id: I7531b9edb763d32b96c58104385349d587de3416
Please use the Bug-Url: field, as defined by the commit template.
--
To view, visit http://gerrit.ovirt.org/8277
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7531b9edb763d32b96c58104385349d587de3416
Gerrit-PatchSet: 6
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
