-
Notifications
You must be signed in to change notification settings - Fork 125
Added Multi-threaded search manager #23
base: master
Are you sure you want to change the base?
Conversation
See my code comments for #21 |
Ah sorry I did not notice the comments! I will check them out and fix what you suggested |
Apropos |
Makes sense. I will do that! |
The ConcurrentTweetManager can be given a number of workers that search simultaneously. Resolves: Mottl#4
967d983
to
c541845
Compare
@Mottl I made changes to what you suggested. Mind having another review? |
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.
What to you think, do we actually need a different class (ConcurrentTweetManager
) for parallel downloading?
bbd72aa
to
9e2987c
Compare
@Mottl I resolved the discussions and implemented the receive buffer callback as requested. Mind having a testrun with:
to see if everything is how you wanted it? EDIT: Im getting 12244 as the count of the tweets fetched with this query, all ordered by time |
9e2987c
to
3731a7d
Compare
@Mottl hey! Have you seen my changes? |
@Mottl sorry to bug you, but what is the state? Is there anything I should do? |
hello. |
Sorry, I'm not understanding. Are you asking how to set the number or worker threads? If so, have a look at the tests I made. |
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'd really like to get this merged
if workers > 1: | ||
got.manager.ConcurrentTweetManager.getTweets(tweetCriteria, receiveBufferConcurrent, debug=debug, worker_count=workers, forceMaxTweets=force_max_tweets) | ||
else: | ||
got.manager.TweetManager.getTweets(tweetCriteria, receiveBuffer, debug=debug) |
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 not use Concurrent even when workers = 1 ?
less code paths less bugs.
|
||
if date_diff_per_worker < timedelta(days=1): | ||
max_workers = int((until_date - since_date) / timedelta(days=1)) | ||
raise ValueError("Too many workers for the time span. Each worker needs at least one day for himself, or" |
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.
s/himself/herself (or itself)
|
||
if tweetCriteria.maxTweets != 0 and not forceMaxTweets: | ||
raise ValueError("Max Tweets is not supported by parallel downloading, since the results can not be ordered" | ||
" by time. If you do not care, you can set forceMaxTweets=True!") |
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.
except it's actually sorted in the sorted() call at the end
The ConcurrentTweetManager can be given a number of
workers that search simultaneously.
Resolves: #9