Maor Lipchuk has posted comments on this change.
Change subject: core: NPEs and wrong persisted information when reusing LUNs
......................................................................
Patch Set 7: (4 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
Line 673: if (!lun.getLunConnections().isEmpty()) {
Line 674: StorageHelperDirector.getInstance().getItem(
Line 675:
lun.getLunConnections().get(0).getstorage_type()).removeLun(lun);
Line 676: } else {
Line 677:
StorageHelperDirector.getInstance().getItem(StorageType.FCP).removeLun(lun);
Please add a comment, to make it more easy to maintain.
Also consider switch the condition to be positive instead of negative (such as
if (lun.getLunConnections().isEmpty())
(I think it is more readable this way)
Line 678: }
Line 679:
Line 680: }
Line 681:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/StorageHelperBase.java
Line 129: final List<LUNs> lunsList =
DbFacade.getInstance().getLunDao().getAllForVolumeGroup(storageDomain.getstorage());
Line 130: int numOfRemovedLuns = 0;
Line 131: for (LUNs lun : lunsList) {
Line 132: if
(DbFacade.getInstance().getDiskLunMapDao().getDiskIdByLunId(lun.getLUN_id()) ==
null) {
Line 133:
DbFacade.getInstance().getLunDao().remove(lun.getLUN_id());
Please refactor DbFacade.getInstance().getLunDao() to a method
Line 134: numOfRemovedLuns++;
Line 135: } else {
Line 136:
DbFacade.getInstance().getLunDao().updateLUNsVolumeGroupId(lun.getLUN_id(), "");
Line 137: }
Line 130: int numOfRemovedLuns = 0;
Line 131: for (LUNs lun : lunsList) {
Line 132: if
(DbFacade.getInstance().getDiskLunMapDao().getDiskIdByLunId(lun.getLUN_id()) ==
null) {
Line 133:
DbFacade.getInstance().getLunDao().remove(lun.getLUN_id());
Line 134: numOfRemovedLuns++;
When do u use the numOfRemovedLuns here?
Why not add a log of the numOfRemovedLuns and also add it when we update the
LUNS volume group
Line 135: } else {
Line 136:
DbFacade.getInstance().getLunDao().updateLUNsVolumeGroupId(lun.getLUN_id(), "");
Line 137: }
Line 138: }
Line 132: if
(DbFacade.getInstance().getDiskLunMapDao().getDiskIdByLunId(lun.getLUN_id()) ==
null) {
Line 133:
DbFacade.getInstance().getLunDao().remove(lun.getLUN_id());
Line 134: numOfRemovedLuns++;
Line 135: } else {
Line 136:
DbFacade.getInstance().getLunDao().updateLUNsVolumeGroupId(lun.getLUN_id(), "");
Consider to use a constant for empty string to have more readable code.
Line 137: }
Line 138: }
Line 139: return numOfRemovedLuns;
Line 140: }
--
To view, visit http://gerrit.ovirt.org/9229
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I38a0e3c68cb8bd80c2f78ee5aacfccc8c987a79e
Gerrit-PatchSet: 7
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
Gerrit-Reviewer: liron aravot <liron.ara...@gmail.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
