-
Notifications
You must be signed in to change notification settings - Fork 36
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
ON HOLD: Tribler channel downloading experiment #282
Conversation
Can one of the admins verify this patch? |
add to test |
ok to test |
add to whitelist |
d79a825
to
31f10e9
Compare
@devos50, I'm pretty much finished with this one. Do you want to review it? Or should we wait for the new gumby to be finished first? |
d0bcc48
to
147962b
Compare
d63afe7
to
ebfdf7b
Compare
ebfdf7b
to
e2a5065
Compare
5c62d09
to
014ce2f
Compare
retest this please |
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 think it looks ok in general (only have a few comments), however, the commit history requires some attention. Please squash it to one or two commits. Also, don't forget to rebase.
After that, I will merge it.
|
||
count_out = 0 | ||
while self._dispersy is None and count_out < 100: | ||
time.sleep(1.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.
Why is this sleep necessary?
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.
Basically, it waits for dispersy to be properly started
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.
Please use reactor.callLater
instead, this will block the whole thing.
|
||
from Tribler.Core.TFTP.handler import TftpHandler | ||
|
||
self.session.lm.tftp_handler = TftpHandler(self.session, self.endpoint, |
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.
Why create a custom TFTP handler here? This one is created when Tribler starts up.
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.
Because I use custom dispersy instance, and I need to put its endpoint on TFTP instance.
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 even need TFTP for your experiment? If not, please remove it.
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.
Yes I need that, indirectly. I used RemoteTorrentHandler.download_torrent
which called TftpRequester
which use TFTP.
try: | ||
os.makedirs(self.upload_dir_path) | ||
except OSError: | ||
# race condition of creating shared directory may happen |
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.
If you want to prevent this, you should make sure that only one process (client) is creating the directory. Or you can Gumby be responsible for the creation of this directory before you start the experiment.
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 condition usually happened in DAS experiment. I couldn't find gumby stuff to handle the directory creation on slave nodes nor I can control which process run in which nodes.
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.
Ah I see the problem. Please document this situation a bit better in the code then please (in the comment) since it might lead to confusion.
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.
Done! If there is no other feedback, I can squash the changes
# writing file has not finished yet | ||
reactor.callLater(10.0, self.setup_seeder, filename, size) | ||
else: | ||
# file not found. In DAS case, this is because the file in another node |
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.
'because the file is in another node'
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.
Fixed!
1d64359
to
d719e4e
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.
Just saw two more things, sorry for the late review!
|
||
self.session = Session(scfg=self.session_config) | ||
|
||
self.session.start() |
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 method returns a deferred, please wait for it using either yield
or by adding a callback.
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 did it a bit differently by now. Check it out
self.session.start() | ||
|
||
while not self.session.lm.initComplete: | ||
time.sleep(0.5) |
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.
Don't use time.sleep
.
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.
removed
a673fd0
to
a2ad1c1
Compare
rebase and review ? |
Squashed commits : - applying TFTP to download from peers - Merge upload and download ds callback - Make sure dispersy running - Add logger.conf - Force connect peer from Dispersy candidates - Delete data when finish experiment - Enable multi-download peer call - Add availability to logs - Stop stalled download - Reduce download speed - Applying post script and logging - Add name id to distinguish from other similar experiment - Add stop download function - Filter local peers to not included in DownloadState - Fix setup seeder race condition - Add set_speed function - Fix publish swarm if its joined any community already - Set to seed as much as possible in both seeder and leecher
Squashed commits : - Add Multiswarm channel experiment
Squashed commits : - Use Tribler's dispersy only if available - Make endpoint reference - Remove obsolete set_videoplayer
a2ad1c1
to
0f860dc
Compare
rebased and ready to review again |
This pull request contains a experiment working on the downloading activity via
ChannelCommunity
.The steps are :
ChannelCommunity
, put this in the AllChannelCommunityChannelCommunity
. At this point, this node already start seedingThis PR based on commit 89510a6 instead of HEAD because of issue #281.
Credit mining experiment will expand from this PR later on. See this and the comment above it.