Change in vdsm[master]: Refactor getFilelist()
Saggi Mizrahi has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 4: (1 comment) File vdsm/BindingXMLRPC.py Line 476: domain = API.StorageDomain(sdUUID) Line 477: return domain.format(autoDetach) Line 478: Line 479: def domainGetFileStats(self, sdUUID, pattern='*', Line 480:caseSensitive=True, options=None): I agree, even though normally this would be the right thing to do due to backward compatibility since you are already changing the name of the verb you are throwing BC out of the door anyway. Line 481: domain = API.StorageDomain(sdUUID) Line 482: return domain.getFileStats(pattern, caseSensitive) Line 483: Line 484: def domainGetImages(self, sdUUID, options=None): -- To view, visit http://gerrit.ovirt.org/20476 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@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
Change in vdsm[master]: Refactor getFilelist()
Saggi Mizrahi has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 4: Removing myself as a reviewer, ping me if the interface file changes -- To view, visit http://gerrit.ovirt.org/20476 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Refactor getFilelist()
Sergey Gotliv has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 4: (12 comments) Commit Message Line 3: AuthorDate: 2013-10-23 23:36:03 +0300 Line 4: Commit: Sergey Gotliv sgot...@redhat.com Line 5: CommitDate: 2013-10-26 15:18:52 +0300 Line 6: Line 7: Refactor getFilelist() Done Line 8: Line 9: 1. Rename to getFileStats() Line 10: Line 11: New name getFileStats() better describes the essence of that API. This Line 10: Line 11: New name getFileStats() better describes the essence of that API. This Line 12: is safe to rename it now because this API is not in use. Line 13: Line 14: 2. Allow to perform case incesitive search Done Line 15: Line 16: Currently Engine is using getIsoList() and getFloppyList() APIs to Line 17: retrieve the list of iso and floppy files respectively. Both these APIs Line 18: are working in case incensitive manner but don't provide file Line 15: Line 16: Currently Engine is using getIsoList() and getFloppyList() APIs to Line 17: retrieve the list of iso and floppy files respectively. Both these APIs Line 18: are working in case incensitive manner but don't provide file Line 19: statistics. Done Line 20: getFileList() is currently not used. It returns file statistics but Line 21: uses case sensitive search. Using case insensitive search Line 22: allow using getFileList() instead of getIsoList() and getFloppyList(). Line 23: Line 18: are working in case incensitive manner but don't provide file Line 19: statistics. Line 20: getFileList() is currently not used. It returns file statistics but Line 21: uses case sensitive search. Using case insensitive search Line 22: allow using getFileList() instead of getIsoList() and getFloppyList(). Done Line 23: Line 24: 3. Rename return value and exception to reflect the change of the API Line 25: name. Line 26: Line 21: uses case sensitive search. Using case insensitive search Line 22: allow using getFileList() instead of getIsoList() and getFloppyList(). Line 23: Line 24: 3. Rename return value and exception to reflect the change of the API Line 25: name. Yes. CaseSensitive is obvious either. :-) Line 26: Line 27: Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Line 28: Bug-Url: https://bugzilla.redhat.com/1005889 File vdsm/BindingXMLRPC.py Line 476: domain = API.StorageDomain(sdUUID) Line 477: return domain.format(autoDetach) Line 478: Line 479: def domainGetFileStats(self, sdUUID, pattern='*', Line 480:caseSensitive=True, options=None): Done Line 481: domain = API.StorageDomain(sdUUID) Line 482: return domain.getFileStats(pattern, caseSensitive) Line 483: Line 484: def domainGetImages(self, sdUUID, options=None): File vdsm/storage/hsm.py Line 2225: return self.taskMng.revertTask(taskID=taskID) Line 2226: Line 2227: @public Line 2228: def getFileStats(self, sdUUID, pattern='*', caseSensitive=True, Line 2229: options=None): There is a match. Now it true, I'll change it to false. Line 2230: Line 2231: Returns statistics of all files in the domain filtered according to Line 2232: pattern. Line 2233: Line 2234: :param sdUUID: The UUID of the storage domain you want to query. Line 2235: :type sdUUID: UUID Line 2236: :param pattern: The glob expression for filtering. Line 2237: :type pattern: str Line 2238: :param caseSensitive: Controls the case of the matching. I think this is so obvious that doesn't need any explanation. This is explanation of IGNORECASE flag in re.py I IGNORECASE Perform case-insensitive matching. This is from javadoc: public static final int CASE_INSENSITIVE Enables case-insensitive matching. Let's see, without these explanations above I would never guess what IGNORECASE means. I will align my explanation with those abobe, but if you'll provide something better, I'll take it. Line 2239: :type caseSensitive: bool Line 2240: :options: ? Line 2241: Line 2242: :returns: a dict of all the volumes found. Line 2238: :param caseSensitive: Controls the case of the matching. Line 2239: :type caseSensitive: bool Line 2240: :options: ? Line 2241: Line 2242: :returns: a dict of all the volumes found. Done Line 2243: :rtype: dict Line 2244: Line 2245: vars.task.setDefaultException(se.GetFileStatsError(sdUUID)) Line 2246: vars.task.getSharedLock(STORAGE, sdUUID) Line 2249: if not dom.isISO or dom.getStorageType() != sd.NFS_DOMAIN: Line 2250: raise se.GetFileStatsError(sdUUID) Line 2251: Line 2252: filesDict = dom.getFileList(pattern=pattern,
Change in vdsm[master]: Refactor getFilelist()
Sergey Gotliv has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 2: After all I decided to define 'caseSensitive' option explicitly instead of using it as part of the options. I checked all available options to use with regex in re.py and didn't find something interesting that we may want to use in the future. I also didn't want to deal with the question of how keep these options so Engine and VDSM will use the same constants like IGNORECASE... -- To view, visit http://gerrit.ovirt.org/20476 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Refactor getFilelist()
Sergey Gotliv has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 2: I aware of the jenkins failure and will fix it later. I am just checking direction. -- To view, visit http://gerrit.ovirt.org/20476 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Refactor getFilelist()
oVirt Jenkins CI Server has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 3: Code-Review-1 Verified-1 Build Failed http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5093/ : UNSTABLE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4289/ : FAILURE http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5167/ : FAILURE -- To view, visit http://gerrit.ovirt.org/20476 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Refactor getFilelist()
Sergey Gotliv has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 4: PEP8 -- To view, visit http://gerrit.ovirt.org/20476 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Refactor getFilelist()
oVirt Jenkins CI Server has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 4: Build Successful http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/5094/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/4290/ : SUCCESS http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/5168/ : SUCCESS -- To view, visit http://gerrit.ovirt.org/20476 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: Ayal Baron aba...@redhat.com Gerrit-Reviewer: Dan Kenigsberg dan...@redhat.com Gerrit-Reviewer: Federico Simoncelli fsimo...@redhat.com Gerrit-Reviewer: Nir Soffer nsof...@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizr...@redhat.com Gerrit-Reviewer: Sergey Gotliv sgot...@redhat.com Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: Refactor getFilelist()
Nir Soffer has posted comments on this change. Change subject: Refactor getFilelist() .. Patch Set 4: (12 comments) Looks good but needs some cleanups. Commit Message Line 3: AuthorDate: 2013-10-23 23:36:03 +0300 Line 4: Commit: Sergey Gotliv sgot...@redhat.com Line 5: CommitDate: 2013-10-26 15:18:52 +0300 Line 6: Line 7: Refactor getFilelist() This is not a refactoring - refactoring is improving the design of the code without changing the behavior. This is change in the behavior. I would describe it as: Replace unused getFileList() with more flexible getFileStats() Now it is very clear what you are doing here. and the rest of the commit message can explain why getFileList needed to be replaced, note that this is a safe change, and describe how getFileStats is more flexible. Line 8: Line 9: 1. Rename to getFileStats() Line 10: Line 11: New name getFileStats() better describes the essence of that API. This Line 10: Line 11: New name getFileStats() better describes the essence of that API. This Line 12: is safe to rename it now because this API is not in use. Line 13: Line 14: 2. Allow to perform case incesitive search The correct term is case-sensitive or case-insensitive. See http://en.wikipedia.org/wiki/Case_sensitivity Maybe: 2. Add caseSensitive option Line 15: Line 16: Currently Engine is using getIsoList() and getFloppyList() APIs to Line 17: retrieve the list of iso and floppy files respectively. Both these APIs Line 18: are working in case incensitive manner but don't provide file Line 15: Line 16: Currently Engine is using getIsoList() and getFloppyList() APIs to Line 17: retrieve the list of iso and floppy files respectively. Both these APIs Line 18: are working in case incensitive manner but don't provide file Line 19: statistics. Replace case incensitive with case insensitive. Line 20: getFileList() is currently not used. It returns file statistics but Line 21: uses case sensitive search. Using case insensitive search Line 22: allow using getFileList() instead of getIsoList() and getFloppyList(). Line 23: Line 18: are working in case incensitive manner but don't provide file Line 19: statistics. Line 20: getFileList() is currently not used. It returns file statistics but Line 21: uses case sensitive search. Using case insensitive search Line 22: allow using getFileList() instead of getIsoList() and getFloppyList(). This was correct in some patches ago; now it should be: Adding caseSensitive option allows using getFileStats() instead of getIsoList() and getFloppyList(). Line 23: Line 24: 3. Rename return value and exception to reflect the change of the API Line 25: name. Line 26: Line 21: uses case sensitive search. Using case insensitive search Line 22: allow using getFileList() instead of getIsoList() and getFloppyList(). Line 23: Line 24: 3. Rename return value and exception to reflect the change of the API Line 25: name. This is obvious. Line 26: Line 27: Change-Id: I550827c7b4c7e11fe09e41745fcc9d91249c6c23 Line 28: Bug-Url: https://bugzilla.redhat.com/1005889 File vdsm/BindingXMLRPC.py Line 476: domain = API.StorageDomain(sdUUID) Line 477: return domain.format(autoDetach) Line 478: Line 479: def domainGetFileStats(self, sdUUID, pattern='*', Line 480:caseSensitive=True, options=None): I think caseSensitive default should be False since this is more useful when doing glob style searches, for example searching ISO files (*.iso or *.ISO). However we should also be consistent; if the underlying getFileList we call use caseSensitive=True, we should use the same default. Line 481: domain = API.StorageDomain(sdUUID) Line 482: return domain.getFileStats(pattern, caseSensitive) Line 483: Line 484: def domainGetImages(self, sdUUID, options=None): File vdsm/storage/hsm.py Line 2225: return self.taskMng.revertTask(taskID=taskID) Line 2226: Line 2227: @public Line 2228: def getFileStats(self, sdUUID, pattern='*', caseSensitive=True, Line 2229: options=None): Match caseSensitive default with BindingXMLRPC.py Line 2230: Line 2231: Returns statistics of all files in the domain filtered according to Line 2232: pattern. Line 2233: Line 2234: :param sdUUID: The UUID of the storage domain you want to query. Line 2235: :type sdUUID: UUID Line 2236: :param pattern: The glob expression for filtering. Line 2237: :type pattern: str Line 2238: :param caseSensitive: Controls the case of the matching. Not clear - check how similar apis describe this feature. Line 2239: :type caseSensitive: bool Line 2240: :options: ? Line 2241: Line 2242:
