Liron Aravot has posted comments on this change.
Change subject: core: added snapshot id to attached disk (#834004)
......................................................................
Patch Set 6: (2 inline comments)
answered mkublin comments.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java
Line 124: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) {
i don't see it as 'we are scared of our shadow'. my opinion is that this IF
statement cost nothing in terms of performance but provides more readability
for developers and helps to avoid Runtime exception in case of unchecked cast.
I don't think that it's right to count on the flow and use the supposed to be
the types - we can get here followed by gui/db/whatever bug, it's kind of the
same as using raw types instead of generic types because we assume that we know
what will be in the collection.
anyway, that's a minor change and here only to provide runtime safety..if you
think that it's better to remove it, i will.
Line 129: diskImage.setvm_snapshot_id(snapshotId);
I don't follow you here - the purpose of the test and the code is to pass when
i push it, if any other developer will change the code and break it (like
putting this line as comment for example)..that test will fail. sorry if i
misunderstood you.
--
To view, visit http://gerrit.ovirt.org/5718
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie0945cecc63dc041fdb56e2fd7b733b287a9904c
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: 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: Michael Kublin <mkub...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
