Nir Soffer has posted comments on this change.

Change subject: Changed InvalidPhysDev to InaccessiblePhysDev InvalidPhysDev 
gave a cryptic error message - Invalid physical device. The message was amended 
into "Multipath cannot access device" along with device name. The error key was 
changed according to the new messa
......................................................................


Patch Set 1:

(4 comments)

The message change looks good, I'm not sure about renaming the exception. These 
are two unrelated changes and the later is not required for fixing the bug.

I suggest to keep this change minimal and only fix the error message for now.

....................................................
Commit Message
Line 3: AuthorDate: 2013-11-05 17:33:36 +0200
Line 4: Commit:     Vered Volansky <vvola...@redhat.com>
Line 5: CommitDate: 2013-11-10 07:47:43 +0200
Line 6: 
Line 7: Changed InvalidPhysDev to InaccessiblePhysDev
The title does not describe the intent of this change. How about:

    exception: Improve error message when multipath cannot access a pv
Line 8: InvalidPhysDev gave a cryptic error message - Invalid physical device.
Line 9: The message was amended into "Multipath cannot access device" along with
Line 10: device name. The error key was changed according to the new message 
content.
Line 11: The error code remains the same.


Line 6: 
Line 7: Changed InvalidPhysDev to InaccessiblePhysDev
Line 8: InvalidPhysDev gave a cryptic error message - Invalid physical device.
Line 9: The message was amended into "Multipath cannot access device" along with
Line 10: device name. The error key was changed according to the new message 
content.
Error key? I think you mean something like:

    The exception was renamed to match the semantics of this error.

Please verify that the exception name is not bubbled up to engine and the 
different name can cause trouble.
Line 11: The error code remains the same.
Line 12: 
Line 13: Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e
Line 14: Bug-url: https://bugzilla.redhat.com/852003


Line 7: Changed InvalidPhysDev to InaccessiblePhysDev
Line 8: InvalidPhysDev gave a cryptic error message - Invalid physical device.
Line 9: The message was amended into "Multipath cannot access device" along with
Line 10: device name. The error key was changed according to the new message 
content.
Line 11: The error code remains the same.
If it did not change, why mention it?
Line 12: 
Line 13: Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e
Line 14: Bug-url: https://bugzilla.redhat.com/852003


....................................................
File vdsm/storage/storage_exception.py
Line 1454: 
Line 1455: 
Line 1456: class InaccessiblePhysDev(StorageException):
Line 1457:     def __init__(self, pvname):
Line 1458:         self.value = "pvname=%s" % (pvname)
I would not :-) Look at other exceptions around this.
Line 1459:     code = 606
Line 1460:     message = "Multipath cannot access storage device"
Line 1461: 
Line 1462: 


-- 
To view, visit http://gerrit.ovirt.org/21089
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I648ee519873c51573e6e6306b79380f54bb25d2e
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky <vvola...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com>
Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
vdsm-patches@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to