Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cachehitrate implies a rate but is used as a boolean flag #3

Open
3 tasks
HerrHorizontal opened this issue Oct 20, 2020 · 0 comments
Open
3 tasks

cachehitrate implies a rate but is used as a boolean flag #3

HerrHorizontal opened this issue Oct 20, 2020 · 0 comments
Labels
refactoring Restructuring for better intelligibility

Comments

@HerrHorizontal
Copy link
Contributor

HerrHorizontal commented Oct 20, 2020

The cachehitrate parameter in RequestedFiles_HitrateBased is currently used as a boolean despite being an integer.

class RequestedFile_HitrateBased(RequestedFile):
"""
Represents a requested file in hitrate based caching.
The cachehitrate flag is somewhat messed up currently.
**Its use should be reworked when remodeling the connection module.**
"""
__slots__ = "cachehitrate"
def __init__(self, filename: str, filesize: int, cachehitrate: int):
super().__init__(filename, filesize)
"""flag whether the file is cached, 1 if it is cached, 0 if it is not cached"""
self.cachehitrate = cachehitrate

Since the meaning of a rate implies a percentage, this either needs a renaming or checking if the application of cachehitrate in terms of a rate does not change expectation, e.g. in the HitrateStorage and FileBasedHitrateStorage implementation.
Here the cachehitrate is used in two different ways,

  • as a boolean in the transfer method for FileBasedHitrateStorage

if file.cachehitrate:
await self.connection.transfer(total=file.filesize)
await sampling_required.put(self.connection)
else:
print("wants to read from remote")
print("file is not cached but cache is file source, this should not occur")
raise ValueError

  • as a multiplicative factor in the find method for FileBasedHitrateStorage

return LookUpInformation(file.filesize * file.cachehitrate, self)

  • as a multiplicative factor in transfer for HitrateStorage (however, this class is out of date already)

hitrate_size = self._hitrate * file.filesize

As the current implementation already ensures correct functioning also with rates, I would propose to actually interpret it as it is named already. This means the following should be done:

  • adaptation of docstrings to remove mentioning of interpretation as a flag
  • adaptation of typehints
  • unit tests to ensure that everything works as expected
@HerrHorizontal HerrHorizontal added invalid This doesn't seem right refactoring Restructuring for better intelligibility labels Oct 20, 2020
@eileen-kuehn eileen-kuehn removed the invalid This doesn't seem right label Oct 29, 2020
@eileen-kuehn eileen-kuehn changed the title [Files] cachehitrate flag is unintuitive, misleading and ambigous cachehitrate flag is unintuitive, misleading and ambigous Jun 21, 2021
@eileen-kuehn eileen-kuehn changed the title cachehitrate flag is unintuitive, misleading and ambigous cachehitrate implies a rate but is used as a boolean flag Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Restructuring for better intelligibility
Projects
None yet
Development

No branches or pull requests

3 participants