Francesco Romani has posted comments on this change.

Change subject: migration: Add retry logic for incoming limit
......................................................................


Patch Set 2:

(2 comments)

https://gerrit.ovirt.org/#/c/46971/2/lib/vdsm/response.py
File lib/vdsm/response.py:

Line 69:         if err:
       :             return code == errCode[err]
       :         else:
I don't like this. Can't you just do

  expected_error = response.error('migrationLimit')
  res = some_call()
  if res == expected_error:
     # handle it

?


https://gerrit.ovirt.org/#/c/46971/2/vdsm/virt/migration.py
File vdsm/virt/migration.py:

Line 346:                     # so don't try again
Line 347:                     if self._migrationCanceledEvt:
Line 348:                         break
Line 349: 
Line 350:                     SourceThread._ongoingMigrations.release()
I understand why you do this but I really don't like it this way. Looks fragile 
and makes the code harder to follow.
Let's think about a cleaner way, possibly involving (the least amount of) 
refactoring.
Line 351:                     # the destination is busy with incoming migrations
Line 352:                     # release semaphore and give other outgoing 
migrations
Line 353:                     # a chance
Line 354:                     time.sleep(5)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Icabe79dfccb61ce43489a9a242a5390e73979649
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Betak <[email protected]>
Gerrit-Reviewer: Francesco Romani <[email protected]>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Betak <[email protected]>
Gerrit-Reviewer: Michal Skrivanek <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
Gerrit-Reviewer: gerrit-hooks <[email protected]>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to