storageService: add new query timeout #1170
Conversation
sevein
added
the
Status: in progress
|
I think I need to review the client module again because I'm not entirely sure that the new timeout setting is used in all places where we could be taking advantage of it. |
qubot
force-pushed
the
dev/issue-1133-ss-quick-timeout
branch
2 times, most recently
from
ccb3ff7 to
8f4a666
Compare
There was a problem hiding this comment.
I have some comments, questions and ideas about the function naming and where the quick timeout could be used.
I think maybe the quick timeout session could be used in session = storage_service._storage_api_session() of failedTransferCleanup.py (and failedSIPCleanup.py) because those PATCH requests just update a Package instance in the db.
| @@ -249,7 +253,7 @@ def copy_files(source_location, destination_location, files): | |||
|
|
|||
| url = _storage_service_url() + 'location/' + destination_location['uuid'] + '/async/' | |||
| try: | |||
| response = _storage_api_session().post(url, json=move_files) | |||
There was a problem hiding this comment.
Unable to put the comment at line 209, but shouldn't _storage_api_session in wait_for_async be _storage_api_quick_session? That should be a quick response too, no?
| @@ -441,7 +446,7 @@ def request_file_deletion(uuid, user_id, user_email, reason_for_deletion): | |||
| 'user_id': user_id, | |||
There was a problem hiding this comment.
Again, unable to comment at actual line, but is there a reason not to use _storage_api_quick_session in get_file_info above?
There was a problem hiding this comment.
As Rich Hickey says, "perception should be free".
There was a problem hiding this comment.
Aw man, get_file_metadata can take a long time because it queries all of the files of a package into memory and json-serializes them. That endpoint needs some pagination desperately. Created artefactual/archivematica-storage-service#389
There was a problem hiding this comment.
My general comment is that _storage_api_session should be the exceptional case and all instances of it should be accompanied by a comment indicating why it is necessary. I think the above comment could be one such code comment.
To take the next nitpicky step given the above, I think the two func names should instead be _storage_api_session and _storage_api_slow_session.
There was a problem hiding this comment.
Ok, maybe all instances where the slow session is used do not need to be commented. For some it may be obvious.
qubot
force-pushed
the
dev/issue-1133-ss-quick-timeout
branch
from
8f4a666 to
7aa7a1f
Compare
sevein
added
Status: review
|
@jrwdunham ok done!
|
This is in addition to the existing client timeout that already exists in the storageService module. This new timeout is used in requests that we know they should be processed immediately. Implementation detail: `_storage_api_session` is now used in requests that are supposed to be processed quickly (including async). A new private function called `_storage_api_slow_session` is used in those requests that still take too long which is undesirable but we haven't had the time to fix yet. This is connected to #1133.
qubot
force-pushed
the
dev/issue-1133-ss-quick-timeout
branch
from
7aa7a1f to
d5976ad
Compare
qubot
removed
the
Status: review
This is in addition to the existing client timeout that already exists in the
storageService module. This new timeout is used in requests that we know they
should be processed immediately.
This is connected to #1133.