-
Notifications
You must be signed in to change notification settings - Fork 7
Job download paging #230
base: master
Are you sure you want to change the base?
Job download paging #230
Conversation
a98499b
to
6e6cc49
Compare
@@ -251,6 +251,9 @@ class Downloader(object): | |||
# record last threads alive | |||
_threads_alive = () | |||
|
|||
# The number of Downloads to fetch per request (only relevant when downloading by job_id) | |||
download_page_size = CONFIG.get("download_page_size", 200) |
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.
The default of 200
is already defined as the default value for page_size
on get_job_downloads
.
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.
not sure what your point is. If there is a value in the CONFIG, we want to use that value (otherwise, use 200). The default page_size
value in the get_job_downloads
function is irrelevant (since we explicitly provide the page_size
argument when making the call).
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.
My point is that we hardcode a default value of 200
in two different places. This is redundant, and because one of the two places is as a function's default value, I think we should remove the default value for the download_page_size
variable and let the function use the default argument value if download_page_size
is None
there: https://github.com/AtomicConductor/conductor_client/pull/230/files#diff-2ce48d04d28c78c28bd96ccc55ae441cR458
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.
We've discussed this before #203 (comment)
I'm not sure what issue concerns you. Is it that it doesn't feel SSOT/DRY?
And are you suggesting that the signature of get_job_downloads
function look like this:
def get_job_downloads(endpoint, client, tids=None, page_size=None):
if page_size == None:
page_size = 200
and therefore omit a default value here:
download_page_size = CONFIG.get("download_page_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.
My concern is that we are hardcoding the same default value in two places, when it doesn't have to be. The default value could be defined as a default value for an argument once, and that would be the end of it. For the sake of avoiding future regressions, we shouldn't hardcode the same default value in more than one place.
I am suggesting this
def get_job_downloads(endpoint, client, tids=None, page_size=200):
and that:
download_page_size = CONFIG.get("download_page_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.
This would result in a page_size
of None
when calling get_job_downloads
. So I'm a little confused how this this solution would work.
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.
Good point. In that case, your suggestion of:
def get_job_downloads(endpoint, client, tids=None, page_size=None):
if page_size == None:
page_size = 200
is preferable IMO.
1a7abdc
to
125d5ec
Compare
- no longer waits for *all* of a job's downloads to be fetched before starting the downloading process, i.e. downloading starts as soon as one download is fetched for the job. - added support for specifying multiple tids when downloading. - the --job_id and --task_id flags now take multiple space-separated args, e.g. --task_id 0 1 3 4
125d5ec
to
9cbc2a1
Compare
No description provided.