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: :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.
volumes?
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,
Line 2253: caseSensitive=caseSensitive)
Replace filesDict with fileStats.
Line 2254: return {'fileStats': filesDict}
Line 2255:
Line 2256: @public
Line 2257: def getIsoList(self, spUUID, extension='iso', options=None):
....................................................
File vdsm/storage/storage_exception.py
Line 606:
Line 607:
Line 608: class GetFileStatsError(StorageException):
Line 609: code = 330
Line 610: message = "Cannot get files stats"
Replace files with file.
Line 611:
Line 612:
Line 613: #################################################
Line 614: # Domains Exceptions
....................................................
File vdsm_api/vdsmapi-schema.json
Line 3840: # @storagedomainID: The UUID of the Storage Domain
Line 3841: #
Line 3842: # @pattern: Filter results by this glob pattern
Line 3843: #
Line 3844: # @caseSensitive: Controls the case of the matching
Not clear - match getFileStats docstring.
Line 3845: #
Line 3846: # Returns:
Line 3847: # A dictionary of file information indexed by file name
Line 3848: #
--
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
