-
Notifications
You must be signed in to change notification settings - Fork 447
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
READY: Implemented endpoint to start a download from infohash #2338
Conversation
(test seems to hang?) |
retest this please |
52cbd00
to
f7130dd
Compare
retest this please |
f7130dd
to
e84803f
Compare
@lfdversluis ready for your review |
Looks good to me. |
@lfdversluis I'm thinking about adding some parameters for anonymous downloading/seeding + destination on disk so I will work on that first (was a little to fast to ask you for review ;) |
88deea5
to
a83c596
Compare
retest this please |
See #2363 |
retest this please |
Tests on mac timed out |
retest this please |
a83c596
to
b9488de
Compare
retest this please |
1 similar comment
retest this please |
The error encountered here is probably fixed by #2392 |
download_config = DownloadStartupConfig() | ||
|
||
has_anon_download = 'anon_download' in parameters and len(parameters['anon_download']) > 0 | ||
has_safe_seeding = 'safe_seeding' in parameters and len(parameters['safe_seeding']) > 0 |
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.
(Laurens): If now someone specifies (anon_download = false), then your code will think it's true. This is more a flag based approach than an argument based approach.
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.
@whirm do you know the best way to make it a flag?
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.
@devos50 I'll come to your office later to talk about this
bffa981
to
0190c68
Compare
retest this please |
Job got aborted halfway? |
retest this please |
|
retest this please |
1 similar comment
retest this please |
|
retest this please |
This one is too unstable too be merged at the moment due to the thread pool stuff when checkpointing (this is probably since I'm starting downloads in the tests I've added). |
I'm working on removing the threadpool altogether, but I'm finding so many bugs while doing it that I spend most of the time fixing them so I can go on. |
retest this please |
@whirm can you review? |
.. sourcecode:: none | ||
|
||
curl -X PUT http://localhost:8085/download/4344503b7e797ebf31582327a5baae35b11bda01 | ||
--data "anon_download=true&safe_seeding=true&destination=/my/dest/on/disk/" |
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 talked about using the hop number instead (0 hops being plain download)
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.
@whirm ah I forgot to update the doc string here. Is that your only comment regarding this PR? :)
retest this please |
0190c68
to
3bcea68
Compare
3bcea68
to
bb5c165
Compare
success :) |
retest this please |
@whirm can you review this already? (see my last two commits) |
Looks fine, let's see if the tests pass |
Also, please send the fixes to next |
Now, only the starting of the DHT is executed on the thread pool. The check itself is executed on the reactor thread.
bb5c165
to
da834ca
Compare
Still the same error on win64... |
retest this please |
This PR is superseded by #2464 |
Shouldn't it be closed then? |
No description provided.