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

[OLD] Respond to mustBeFresh Interests #96

Closed
wants to merge 4 commits into from

Conversation

a-thieme
Copy link
Contributor

@a-thieme a-thieme commented Oct 5, 2024

The repo now responds to mustBeFresh Interests, following the same logic as the Content Store in NFD

Lots of code cleanup

Copy link
Collaborator

@zjkmxy zjkmxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functionality-wise looks good to me.
I have some stylish comments.

@@ -188,9 +188,10 @@ async def _wait_for_finish(self, check_prefix: NonStrictName, request_no: bytes)

:param check_prefix: NonStrictName. The prefix under which the check message will be\
published.
:param process_id: int. The process id to check.
:param request_no: int. The request number to check.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is of bytes type, not int.

from . import ReadHandle, CommandHandle
from ..command import RepoCommandRes, RepoCommandParam, SyncParam, SyncStatus, RepoStatCode
from ..utils import concurrent_fetcher, PubSub, PassiveSvs, IdNamingConv
from ..storage import Storage
from typing import Optional, Tuple, List, Dict
from typing import Dict
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Dict can also be removed. Python 3.9+ allows to use dict as the type name without importing anything.

"""
with self.db.write_batch() as b:
for key, value, expire_time_ms in zip(keys, values, expire_time_mss):
b.put(key, pickle.dumps((value, expire_time_ms)))

def _get(self, key: bytes, can_be_prefix=False, must_be_fresh=False) -> bytes:
def _get(self, key: bytes, can_be_prefix=False, must_be_fresh=False) -> Any | None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any is vague: what is the actual type of the return value?
If you do not know exactly, you can remove the type hint.

@@ -118,7 +120,7 @@ def get_data_packet(self, name: NonStrictName, can_be_prefix: bool=False,
while True:
data, expire_time_ms = next(it)
if not must_be_fresh or expire_time_ms > self._time_ms():
self.logger.info('get from cache')
self.logger.info(f'get from cache')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Some linter will complain if you put an f prefix to a string literal without {}.

try:
task_num = int(task_name)
except: continue
except: # fixme: what specific error does this catch?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aio.all_tasks returns tasks not created with this module. They have internal names generated by Python.
BTW I don't think it is a good idea to use task names.

@@ -52,7 +52,7 @@ def test_main(self):
def startup(self):
self.files_to_cleanup = []

tmp_cfg_path = self.create_tmp_cfg()
tmp_cfg_path = self.create_tmp_cfg(self)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you pass self to a static method?
If there is no special need for accessing the instance, just remove the argument.

@a-thieme
Copy link
Contributor Author

a-thieme commented Oct 9, 2024

Functionality-wise looks good to me. I have some stylish comments.

I'll split this into two, one for the code quality and one for the mustBeFresh to make things easier

@a-thieme a-thieme closed this Oct 10, 2024
@a-thieme a-thieme deleted the mustBeFresh branch October 10, 2024 20:23
@a-thieme a-thieme changed the title Respond to mustBeFresh Interests [OLD] Respond to mustBeFresh Interests Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants