Ala Hino has posted comments on this change.

Change subject: Live Merge: teardown volume on HSM after live merge
......................................................................


Patch Set 2:

(3 comments)

https://gerrit.ovirt.org/#/c/64301/2/vdsm/virt/vm.py
File vdsm/virt/vm.py:

Line 4769
Line 4770
Line 4771
Line 4772
Line 4773
> Another place where we should not pass an argument.
separate patch ...


Line 4763:         self.vm._setVolumeSize(self.drive.domainID, 
self.drive.poolID,
Line 4764:                                self.drive.imageID, baseVolUUID,
Line 4765:                                topVolInfo['capacity'])
Line 4766: 
Line 4767:     def teardown_top_volume(self, imgUUID, volUUID):
> Don't pass arguments to this method, it make the code  harder to read, and 
Done
Line 4768:         # TODO move this method to storage public API
Line 4769:         sd_manifest = 
sdc.sdCache.produce_manifest(self.drive.domainID)
Line 4770:         sd_manifest.teardownVolume(imgUUID, volUUID)
Line 4771: 


Line 4781:             self.vm.enableDriveMonitor()
Line 4782:         self.success = True
Line 4783:         self.vm.log.info("Synchronization completed (job %s)",
Line 4784:                          self.job['jobID'])
Line 4785:         self.teardown_top_volume(self.drive.imageID, 
self.job['topVolume'])
> This would be much nicer as:
Done
Line 4786: 
Line 4787:     def isSuccessful(self):
Line 4788:         """
Line 4789:         Returns True if this phase completed successfully.


-- 
To view, visit https://gerrit.ovirt.org/64301
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iec3b6adb50293d8c98f5d8726d668eb272d16549
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ala Hino <ah...@redhat.com>
Gerrit-Reviewer: Adam Litke <ali...@redhat.com>
Gerrit-Reviewer: Ala Hino <ah...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org
To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org

Reply via email to