-
Notifications
You must be signed in to change notification settings - Fork 8
Cold storage interface and service #153
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
base: main
Are you sure you want to change the base?
Conversation
b4b1244
to
65bd08a
Compare
2aa4716
to
cb92e84
Compare
return rb | ||
|
||
@staticmethod | ||
def start(req, num_files, size): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def start(req, num_files, size): | |
def start_processing(req, num_files, size): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be better 'started'?
): | ||
self._transfer.append(entry_point.load()()) | ||
else: | ||
self._cold_path = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep this in the database instead of the entrypoints/code
…iles, see the requests and subscribe
886f413
to
5e4caab
Compare
183d001
to
2279ec0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments but nothing major so that cannot be addressed or ticketized for a next iteration on the relevant code.
if tag: | ||
info["tags"][tagName] = tag.value | ||
if "availability" in self.data: | ||
del self.data["availability"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth checking before merging?
avl = "ready" | ||
for t in self.obj.tags: | ||
if t.key == "hot_deleted": | ||
avl = "needs request" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avl = "needs request" | |
avl = "needs_request" |
minor: You can add these in the next iteration but I would try to avoid these hardcoded values and create enums instead.
return transfer | ||
|
||
@staticmethod | ||
def _get_ongoing_transfers(last_check): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems contradictory to have a staticmethod that is private....
def _get_ongoing_transfers(last_check): | |
def get_ongoing_transfers(last_check): |
) | ||
|
||
@staticmethod | ||
def _load_class(full_class_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above...
return current_app.config["COLD_ACTIVE_ARCHIVING_TRANSFERS_THRESHOLD"] | ||
|
||
@staticmethod | ||
def _send_email(req, emails): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _send_email(req, emails): | |
def send_email(req, emails): |
db.session.add(req) | ||
|
||
@staticmethod | ||
def complete(req): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def complete(req): | |
def mark_as_completed(req): |
return True | ||
|
||
@staticmethod | ||
def subscribe(transfer_id, email): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: move to Transfer class or utility?
|
||
By default, it prints the urls for all the files of the entry. | ||
""" | ||
m = ColdStorageManager(current_app) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to pass different Flask application to the class? If not, I think current_app can be accessed directly from the class it needs it...
RequestService._check_running() | ||
|
||
@staticmethod | ||
def _check_submitted(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _check_submitted(): | |
def check_submitted(): |
db.session.commit() | ||
|
||
@staticmethod | ||
def _check_running(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _check_running(): | |
def check_running(): |
No description provided.