Change in vdsm[master]: Refactor v2v jobs for reusability

fromani at redhat.com fromani at redhat.com
Wed Aug 19 13:42:22 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: Refactor v2v jobs for reusability
......................................................................


Patch Set 3:

(4 comments)

initial comments, partial review, no score. Overall looks good, though

https://gerrit.ovirt.org/#/c/44857/3/vdsm/jobs.py
File vdsm/jobs.py:

Line 120:     with _lock:
Line 121:         _jobs.clear()
Line 122: 
Line 123: 
Line 124: def delete_job(job_id):
jobs.delete_job() looks redundant
(same for
jobs.abort_job(),
jobs.get_jobs_status(),
jobs.add_job(),
jobs.get_job(),
)

maybe time to drop the _job in the public function names. What do you think?
Line 125:     try:
Line 126:         job = get_job(job_id)
Line 127:         job.validate_not_running()
Line 128:         _delete_job(job_id)


Line 177:         del _jobs[job_id]
Line 178: 
Line 179: 
Line 180: # Automatically initialize on first import
Line 181: clear()
I prefer explicit initialization, but let's see what the other reviewers like.


https://gerrit.ovirt.org/#/c/44857/3/vdsm/v2v.py
File vdsm/v2v.py:

Line 107: 
Line 108: def get_jobs_status():
Line 109:     all_jobs = jobs.get_jobs_status()
Line 110:     return {job_id: status for (job_id, status) in all_jobs.items()
Line 111:             if status['job_type'] == _V2V_JOB_TYPE}
nice :)
Line 112: 
Line 113: 
Line 114: def get_external_vms(uri, username, password):
Line 115:     if not supported():


Line 232:     def __init__(self, vminfo, job_id, irs):
Line 233:         '''
Line 234:         do not use directly, use a factory method instead!
Line 235:         '''
Line 236:         jobs.Job.__init__(self, job_id)
why not super()?
Line 237:         self._vminfo = vminfo
Line 238:         self._irs = irs
Line 239: 
Line 240:         self._disk_progress = 0


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ida6b1c460c5030c820c540e836e423d4632410df
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list